* Re:[PATCH] PCI: add PCI Express Port Bus Driver subsystem
@ 2005-01-18 19:28 long
2005-01-18 18:36 ` [PATCH] " Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: long @ 2005-01-18 19:28 UTC (permalink / raw)
To: greg; +Cc: linux-kernel, tom.l.nguyen
On Mon, 17 Jan 2005 15:49:08 -0800 Greg KH wrote:
> > +int pcie_port_device_register(struct pci_dev *dev)
> > +{
> > + struct pcie_device *parent;
> > + int status, type, capabilities, irq_mode, i;
> > + int vectors[PCIE_PORT_DEVICE_MAXSERVICES];
> > + u16 reg16;
> > +
> > + /* Get port type */
> > + pci_read_config_word(dev,
> > + pci_find_capability(dev, PCI_CAP_ID_EXP) +
> > + PCIE_CAPABILITIES_REG, ®16);
> > + type = (reg16 >> 4) & PORT_TYPE_MASK;
> > +
> > + /* Now get port services */
> > + capabilities = get_port_device_capability(dev);
> > + irq_mode = assign_interrupt_mode(dev, vectors, capabilities);
> > +
> > + /* Allocate parent */
> > + parent = alloc_pcie_device(NULL, dev, type, 0, dev->irq, irq_mode);
> > + if (!parent)
> > + return -ENOMEM;
> > +
> > + status = device_register(&parent->device);
> > + if (status) {
> > + kfree(parent);
> > + return status;
> > + }
>
>
> This puts all of the pcie "port" structures in /sys/devices/ Shouldn't
> you make the parent of the device you create point to the pci_dev
> structure that's passed into this function? That would make the sysfs
> tree a lot saner I think.
The patch makes the parent of the device point to the pci_dev structure
that is passed into this function. If you think it is cleaner that the
patch should not, I will update the patch to reflect your input.
Thanks,
Long
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] PCI: add PCI Express Port Bus Driver subsystem
2005-01-18 19:28 Re:[PATCH] PCI: add PCI Express Port Bus Driver subsystem long
@ 2005-01-18 18:36 ` Greg KH
0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2005-01-18 18:36 UTC (permalink / raw)
To: long; +Cc: linux-kernel, tom.l.nguyen
On Tue, Jan 18, 2005 at 11:28:57AM -0800, long wrote:
> On Mon, 17 Jan 2005 15:49:08 -0800 Greg KH wrote:
> > > +int pcie_port_device_register(struct pci_dev *dev)
> > > +{
> > > + struct pcie_device *parent;
> > > + int status, type, capabilities, irq_mode, i;
> > > + int vectors[PCIE_PORT_DEVICE_MAXSERVICES];
> > > + u16 reg16;
> > > +
> > > + /* Get port type */
> > > + pci_read_config_word(dev,
> > > + pci_find_capability(dev, PCI_CAP_ID_EXP) +
> > > + PCIE_CAPABILITIES_REG, ®16);
> > > + type = (reg16 >> 4) & PORT_TYPE_MASK;
> > > +
> > > + /* Now get port services */
> > > + capabilities = get_port_device_capability(dev);
> > > + irq_mode = assign_interrupt_mode(dev, vectors, capabilities);
> > > +
> > > + /* Allocate parent */
> > > + parent = alloc_pcie_device(NULL, dev, type, 0, dev->irq, irq_mode);
> > > + if (!parent)
> > > + return -ENOMEM;
> > > +
> > > + status = device_register(&parent->device);
> > > + if (status) {
> > > + kfree(parent);
> > > + return status;
> > > + }
> >
> >
> > This puts all of the pcie "port" structures in /sys/devices/ Shouldn't
> > you make the parent of the device you create point to the pci_dev
> > structure that's passed into this function? That would make the sysfs
> > tree a lot saner I think.
>
> The patch makes the parent of the device point to the pci_dev structure
> that is passed into this function. If you think it is cleaner that the
> patch should not, I will update the patch to reflect your input.
That would be great, but it doesn't show up that way on my box. All of
the portX devices are in /sys/devices/ which is what I don't think you
want. I would love for them to have the parent of the pci_dev structure
:)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re:[PATCH] PCI: add PCI Express Port Bus Driver subsystem
@ 2005-01-19 1:59 long
0 siblings, 0 replies; 4+ messages in thread
From: long @ 2005-01-19 1:59 UTC (permalink / raw)
To: greg; +Cc: linux-kernel, tom.l.nguyen
On Tue Jan 18 11:41:01 2005 Greg KH wrote:
>> >
>> >
>> > This puts all of the pcie "port" structures in /sys/devices/ Shouldn't
>> > you make the parent of the device you create point to the pci_dev
>> > structure that's passed into this function? That would make the sysfs
>> > tree a lot saner I think.
>>
>> The patch makes the parent of the device point to the pci_dev structure
>> that is passed into this function. If you think it is cleaner that the
>> patch should not, I will update the patch to reflect your input.
>
>That would be great, but it doesn't show up that way on my box. All of
>the portX devices are in /sys/devices/ which is what I don't think you
>want. I would love for them to have the parent of the pci_dev structure
>:)
Agree. Thanks for your inputs. The patch below include the changes based
on your previous post.
Signed-off-by: T. Long Nguyen <tom.l.nguyen@intel.com>
---------------------------------------------------------------------
diff -urpN greg-patch/drivers/pci/pcie/portdrv_bus.c greg-patch-update/drivers/pci/pcie/portdrv_bus.c
--- greg-patch/drivers/pci/pcie/portdrv_bus.c 2005-01-18 15:59:38.000000000 -0500
+++ greg-patch-update/drivers/pci/pcie/portdrv_bus.c 2005-01-18 16:03:47.000000000 -0500
@@ -14,8 +14,6 @@
#include <linux/pcieport_if.h>
-static int generic_probe (struct device *dev) { return 0;}
-static int generic_remove (struct device *dev) { return 0;}
static int pcie_port_bus_match(struct device *dev, struct device_driver *drv);
static int pcie_port_bus_suspend(struct device *dev, u32 state);
static int pcie_port_bus_resume(struct device *dev);
@@ -27,23 +25,14 @@ struct bus_type pcie_port_bus_type = {
.resume = pcie_port_bus_resume,
};
-struct device_driver pcieport_generic_driver = {
- .name = "pcieport",
- .bus = &pcie_port_bus_type,
- .probe = generic_probe,
- .remove = generic_remove,
-};
-
static int pcie_port_bus_match(struct device *dev, struct device_driver *drv)
{
struct pcie_device *pciedev;
struct pcie_port_service_driver *driver;
- if ( drv->bus != &pcie_port_bus_type ||
- dev->bus != &pcie_port_bus_type ||
- drv == &pcieport_generic_driver) {
+ if (drv->bus != &pcie_port_bus_type || dev->bus != &pcie_port_bus_type)
return 0;
- }
+
pciedev = to_pcie_device(dev);
driver = to_service_driver(drv);
if ( (driver->id_table->vendor != PCI_ANY_ID &&
diff -urpN greg-patch/drivers/pci/pcie/portdrv_core.c greg-patch-update/drivers/pci/pcie/portdrv_core.c
--- greg-patch/drivers/pci/pcie/portdrv_core.c 2005-01-18 15:59:38.000000000 -0500
+++ greg-patch-update/drivers/pci/pcie/portdrv_core.c 2005-01-18 16:06:51.000000000 -0500
@@ -17,8 +17,6 @@
extern int pcie_mch_quirk; /* MSI-quirk Indicator */
-extern struct device_driver pcieport_generic_driver;
-
static int pcie_port_probe_service(struct device *dev)
{
struct pcie_device *pciedev;
@@ -103,6 +101,7 @@ static int pcie_port_resume_service(stru
*/
static void release_pcie_device(struct device *dev)
{
+ printk(KERN_DEBUG "Free Port Service[%s]\n", dev->bus_id);
kfree(to_pcie_device(dev));
}
@@ -217,18 +216,18 @@ static int get_port_device_capability(st
return services;
}
-static void pcie_device_init(struct pcie_device *parent,
- struct pcie_device *dev,
- int port_type, int service_type)
+static void pcie_device_init(struct pci_dev *parent, struct pcie_device *dev,
+ int port_type, int service_type, int irq, int irq_mode)
{
struct device *device;
- if (parent) {
- dev->id.vendor = parent->port->vendor;
- dev->id.device = parent->port->device;
- dev->id.port_type = port_type;
- dev->id.service_type = (1 << service_type);
- }
+ dev->port = parent;
+ dev->interrupt_mode = irq_mode;
+ dev->irq = irq;
+ dev->id.vendor = parent->vendor;
+ dev->id.device = parent->device;
+ dev->id.port_type = port_type;
+ dev->id.service_type = (1 << service_type);
/* Initialize generic device interface */
device = &dev->device;
@@ -240,35 +239,23 @@ static void pcie_device_init(struct pcie
device->driver = NULL;
device->driver_data = NULL;
device->release = release_pcie_device; /* callback to free pcie dev */
- sprintf(&device->bus_id[0], "%s.%02x", parent->device.bus_id,
- get_descriptor_id(port_type, service_type));
- device->parent = ((parent == NULL) ? NULL : &parent->device);
+ sprintf(&device->bus_id[0], "pcie%02x",
+ get_descriptor_id(port_type, service_type));
+ device->parent = &parent->dev;
}
-static struct pcie_device* alloc_pcie_device(
- struct pcie_device *parent, struct pci_dev *bridge,
+static struct pcie_device* alloc_pcie_device(struct pci_dev *parent,
int port_type, int service_type, int irq, int irq_mode)
{
struct pcie_device *device;
- static int NR_PORTS = 0;
device = kmalloc(sizeof(struct pcie_device), GFP_KERNEL);
if (!device)
return NULL;
memset(device, 0, sizeof(struct pcie_device));
- device->port = bridge;
- device->interrupt_mode = irq_mode;
- device->irq = irq;
- if (!parent) {
- pcie_device_init(NULL, device, port_type, service_type);
- NR_PORTS++;
- device->device.driver = &pcieport_generic_driver;
- sprintf(&device->device.bus_id[0], "port%d", NR_PORTS);
- } else {
- pcie_device_init(parent, device, port_type, service_type);
- }
- printk(KERN_DEBUG "Allocate Port Device[%s]\n", device->device.bus_id);
+ pcie_device_init(parent, device, port_type, service_type, irq,irq_mode);
+ printk(KERN_DEBUG "Allocate Port Service[%s]\n", device->device.bus_id);
return device;
}
@@ -291,7 +278,6 @@ int pcie_port_device_probe(struct pci_de
int pcie_port_device_register(struct pci_dev *dev)
{
- struct pcie_device *parent;
int status, type, capabilities, irq_mode, i;
int vectors[PCIE_PORT_DEVICE_MAXSERVICES];
u16 reg16;
@@ -306,27 +292,13 @@ int pcie_port_device_register(struct pci
capabilities = get_port_device_capability(dev);
irq_mode = assign_interrupt_mode(dev, vectors, capabilities);
- /* Allocate parent */
- parent = alloc_pcie_device(NULL, dev, type, 0, dev->irq, irq_mode);
- if (!parent)
- return -ENOMEM;
-
- status = device_register(&parent->device);
- if (status) {
- kfree(parent);
- return status;
- }
- get_device(&parent->device);
- pci_set_drvdata(dev, parent);
-
/* Allocate child services if any */
for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
struct pcie_device *child;
if (capabilities & (1 << i)) {
child = alloc_pcie_device(
- parent, /* parent */
- dev, /* Root/Upstream/Downstream */
+ dev, /* parent */
type, /* port type */
i, /* service type */
vectors[i], /* irq */
@@ -345,17 +317,21 @@ int pcie_port_device_register(struct pci
}
#ifdef CONFIG_PM
-int pcie_port_device_suspend(struct pcie_device *dev, u32 state)
+int pcie_port_device_suspend(struct pci_dev *dev, u32 state)
{
- struct list_head *head;
+ struct list_head *head, *tmp;
struct device *parent, *child;
struct device_driver *driver;
struct pcie_port_service_driver *service_driver;
- parent = &dev->device;
+ parent = &dev->dev;
head = &parent->children;
- while (!list_empty(head)) {
- child = container_of(head->next, struct device, node);
+ tmp = head->next;
+ while (head != tmp) {
+ child = container_of(tmp, struct device, node);
+ tmp = tmp->next;
+ if (child->bus != &pcie_port_bus_type)
+ continue;
driver = child->driver;
if (!driver)
continue;
@@ -366,17 +342,21 @@ int pcie_port_device_suspend(struct pcie
return 0;
}
-int pcie_port_device_resume(struct pcie_device *dev)
+int pcie_port_device_resume(struct pci_dev *dev)
{
- struct list_head *head;
+ struct list_head *head, *tmp;
struct device *parent, *child;
struct device_driver *driver;
struct pcie_port_service_driver *service_driver;
- parent = &dev->device;
+ parent = &dev->dev;
head = &parent->children;
- while (!list_empty(head)) {
- child = container_of(head->next, struct device, node);
+ tmp = head->next;
+ while (head != tmp) {
+ child = container_of(tmp, struct device, node);
+ tmp = tmp->next;
+ if (child->bus != &pcie_port_bus_type)
+ continue;
driver = child->driver;
if (!driver)
continue;
@@ -389,45 +369,46 @@ int pcie_port_device_resume(struct pcie_
}
#endif
-void pcie_port_device_remove(struct pcie_device *dev)
+void pcie_port_device_remove(struct pci_dev *dev)
{
- struct list_head *head;
+ struct list_head *head, *tmp;
struct device *parent, *child;
struct device_driver *driver;
struct pcie_port_service_driver *service_driver;
+ int interrupt_mode = PCIE_PORT_INTx_MODE;
- parent = &dev->device;
+ parent = &dev->dev;
head = &parent->children;
- while (!list_empty(head)) {
- child = container_of(head->next, struct device, node);
+ tmp = head->next;
+ while (head != tmp) {
+ child = container_of(tmp, struct device, node);
+ tmp = tmp->next;
+ if (child->bus != &pcie_port_bus_type)
+ continue;
driver = child->driver;
if (driver) {
service_driver = to_service_driver(driver);
if (service_driver->remove)
service_driver->remove(to_pcie_device(child));
}
+ interrupt_mode = (to_pcie_device(child))->interrupt_mode;
put_device(child);
device_unregister(child);
}
-
/* Switch to INTx by default if MSI enabled */
- if (dev->interrupt_mode == PCIE_PORT_MSIX_MODE)
- pci_disable_msix(dev->port);
- else if (dev->interrupt_mode == PCIE_PORT_MSI_MODE)
- pci_disable_msi(dev->port);
- put_device(parent);
- device_unregister(parent);
+ if (interrupt_mode == PCIE_PORT_MSIX_MODE)
+ pci_disable_msix(dev);
+ else if (interrupt_mode == PCIE_PORT_MSI_MODE)
+ pci_disable_msi(dev);
}
void pcie_port_bus_register(void)
{
bus_register(&pcie_port_bus_type);
- driver_register(&pcieport_generic_driver);
}
void pcie_port_bus_unregister(void)
{
- driver_unregister(&pcieport_generic_driver);
bus_unregister(&pcie_port_bus_type);
}
diff -urpN greg-patch/drivers/pci/pcie/portdrv.h greg-patch-update/drivers/pci/pcie/portdrv.h
--- greg-patch/drivers/pci/pcie/portdrv.h 2005-01-18 15:59:38.000000000 -0500
+++ greg-patch-update/drivers/pci/pcie/portdrv.h 2005-01-18 16:08:33.000000000 -0500
@@ -28,14 +28,13 @@
#define get_descriptor_id(type, service) (((type - 4) << 4) | service)
extern struct bus_type pcie_port_bus_type;
-extern struct device_driver pcieport_generic_driver;
extern int pcie_port_device_probe(struct pci_dev *dev);
extern int pcie_port_device_register(struct pci_dev *dev);
#ifdef CONFIG_PM
-extern int pcie_port_device_suspend(struct pcie_device *dev, u32 state);
-extern int pcie_port_device_resume(struct pcie_device *dev);
+extern int pcie_port_device_suspend(struct pci_dev *dev, u32 state);
+extern int pcie_port_device_resume(struct pci_dev *dev);
#endif
-extern void pcie_port_device_remove(struct pcie_device *dev);
+extern void pcie_port_device_remove(struct pci_dev *dev);
extern void pcie_port_bus_register(void);
extern void pcie_port_bus_unregister(void);
diff -urpN greg-patch/drivers/pci/pcie/portdrv_pci.c greg-patch-update/drivers/pci/pcie/portdrv_pci.c
--- greg-patch/drivers/pci/pcie/portdrv_pci.c 2005-01-18 15:59:38.000000000 -0500
+++ greg-patch-update/drivers/pci/pcie/portdrv_pci.c 2005-01-18 16:39:07.000000000 -0500
@@ -63,34 +63,18 @@ static int __devinit pcie_portdrv_probe
static void pcie_portdrv_remove (struct pci_dev *dev)
{
- struct pcie_device *pciedev;
-
- pciedev = (struct pcie_device *)pci_get_drvdata(dev);
- if (pciedev) {
- pcie_port_device_remove(pciedev);
- pci_set_drvdata(dev, NULL);
- }
+ pcie_port_device_remove(dev);
}
#ifdef CONFIG_PM
static int pcie_portdrv_suspend (struct pci_dev *dev, u32 state)
{
- struct pcie_device *pciedev;
-
- pciedev = (struct pcie_device *)pci_get_drvdata(dev);
- if (pciedev)
- pcie_port_device_suspend(pciedev, state);
- return 0;
+ return pcie_port_device_suspend(dev, state);
}
static int pcie_portdrv_resume (struct pci_dev *dev)
{
- struct pcie_device *pciedev;
-
- pciedev = (struct pcie_device *)pci_get_drvdata(dev);
- if (pciedev)
- pcie_port_device_resume(pciedev);
- return 0;
+ return pcie_port_device_resume(dev);
}
#endif
^ permalink raw reply [flat|nested] 4+ messages in thread* Re:[PATCH] PCI: add PCI Express Port Bus Driver subsystem
@ 2005-01-24 20:10 long
0 siblings, 0 replies; 4+ messages in thread
From: long @ 2005-01-24 20:10 UTC (permalink / raw)
To: greg; +Cc: linux-kernel, tom.l.nguyen
On Tuesday, January 18, 2005 5:03 PM Greg KH wrote:
>> >
>> >That would be great, but it doesn't show up that way on my box. All
>> >of
>> >the portX devices are in /sys/devices/ which is what I don't think
>> >you
>> >want. I would love for them to have the parent of the pci_dev
>> >structure
>> >:)
>>
>> Agree. Thanks for your inputs. The patch below include the changes
>> based on your previous post.
>
>Hm, that seems like a pretty big patch just to add a pointer to a parent
>device :)
>
>What really does this patch do? What does the sysfs tree now look like?
Before changes:
The patch makes the parent of the device pointing to the pci_dev
structure. The parents portX devices are in /sys/devices which
should be removed based on your suggestions. Below is /sys/devices
before any changes made.
/sys/devices
|
__ ide0
|
__ pci0000:00
|
__ pnp0
|
__ port1
| |
| __ port1.00
| |
| __ port1.01
| .
| .
| .
|
__ port2
|
__ port3
|
__ system
After changes:
The parents portX devices are no longer necessary because port1.00
and port1.01 devices shoud have the parent of the pci_dev structure
(based on your suggestion). The patch does the following changes:
- remove code creating and handling the parent portX devices.
- rename portX.YZ to pcieYZ (for example port1.00 renamed to pcie00)
since portX is no longer needed.
- make pcieYZ have the parent of the pci_dev structure.
Below is /sys/devices after changes made to the patch.
/sys/devices
|
__ ide0
|
__ pci0000:00
| |
| __ 0000:00:00.0
| |
| __ 0000:00:04.0
| | |
| . __ class
| . |
| . __ pcie00
| |
| __ pcie01
| .
| .
| .
|
__ platform
|
__ pnp0
|
__ system
Please let me know what you think of the changes.
Thanks,
Long
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-01-24 19:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-01-18 19:28 Re:[PATCH] PCI: add PCI Express Port Bus Driver subsystem long
2005-01-18 18:36 ` [PATCH] " Greg KH
2005-01-19 1:59 long
2005-01-24 20:10 long
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).