I don't have access to a Sun HME card, but, for some neurotic
reason, I decided to try porting sunhme.c to the new PCI interface.
I believe it has simplified the code slightly. More importantly, it
causes the module to export a table with the PCI ID's that it cares
about, which is used by depmod to generate /lib/modules/.../modules.pcimap,
which can be used to support automatic loading (I wrote a small
program for this purpose, "pcimodules", which I have submitted
hopefully for inclusion in pciutils, and I'd be glad to provide
that patch if anyone is interested).
The patch also eliminates some places where a struct netdevice
that was *always* null was being passed around. Also note that, under
this patch, root_happy_dev has become a list of only the sbus interfaces.
Anyhow, I know that this compiles without any complaint other
than Dave Miller's built in warning message ("This needs to be corrected
... -DaveM"). I would appreciate it if you or anyone else interested
would test it and, if it works and is generally approved of, then
have Dave bless it and send it to Linus.
It looks like sunhme was one of only seven drivers remaining
in the drivers/net (excluding subdirectories) that used the old PCI
probing interface.
This patch is against 2.4.0-test11-pre5, and includes Dave's
changes to make it compile.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
--- linux-2.4.0-test11-pre5/drivers/net/sunhme.c Wed Nov 15 22:07:59 2000
+++ linux/drivers/net/sunhme.c Thu Nov 16 00:07:15 2000
@@ -38,6 +38,7 @@
#include <asm/dma.h>
#include <linux/errno.h>
#include <asm/byteorder.h>
+#include <asm/uaccess.h>
#ifdef __sparc__
#include <asm/idprom.h>
@@ -48,7 +49,6 @@
#ifndef __sparc_v9__
#include <asm/io-unit.h>
#endif
-#include <asm/uaccess.h>
#endif
#include <asm/pgtable.h>
@@ -73,9 +73,8 @@
/* accept MAC address of the form macaddr=0x08,0x00,0x20,0x30,0x40,0x50 */
MODULE_PARM(macaddr, "6i");
-static struct happy_meal *root_happy_dev = NULL;
-
#ifdef CONFIG_SBUS
+static struct happy_meal *root_happy_dev = NULL;
static struct quattro *qfe_sbus_list = NULL;
#endif
@@ -101,6 +100,7 @@
unsigned int status;
};
#define TX_LOG_LEN 128
+
static struct hme_tx_logent tx_log[TX_LOG_LEN];
static int txlog_cur_entry = 0;
static __inline__ void tx_add_log(struct happy_meal *hp, unsigned int a, unsigned int s)
@@ -1600,6 +1600,10 @@
HMD(("happy_meal_init: old[%08x] bursts<",
hme_read32(hp, gregs + GREG_CFG)));
+#ifndef __sparc__
+ /* It is always PCI and can handle 64byte bursts. */
+ hme_write32(hp, gregs + GREG_CFG, GREG_CFG_BURST64);
+#else
if ((hp->happy_bursts & DMA_BURST64) &&
((hp->happy_flags & HFLAG_PCI) != 0
#ifdef CONFIG_SBUS
@@ -1633,6 +1637,7 @@
HMD(("XXX>"));
hme_write32(hp, gregs + GREG_CFG, 0);
}
+#endif /* __sparc__ */
/* Turn off interrupts we do not want to hear. */
HMD((", enable global interrupts, "));
@@ -2553,10 +2558,9 @@
#endif /* CONFIG_PCI */
#ifdef CONFIG_SBUS
-static int __init happy_meal_sbus_init(struct net_device *dev,
- struct sbus_dev *sdev,
- int is_qfe)
+static int __init happy_meal_sbus_init(struct sbus_dev *sdev, int is_qfe)
{
+ struct net_device *dev;
struct quattro *qp = NULL;
struct happy_meal *hp;
int i, qfe_slot = -1;
@@ -2571,13 +2575,7 @@
if (qfe_slot == 4)
return -ENODEV;
}
- if (dev == NULL) {
- dev = init_etherdev(0, sizeof(struct happy_meal));
- } else {
- dev->priv = kmalloc(sizeof(struct happy_meal), GFP_KERNEL);
- if (dev->priv == NULL)
- return -ENOMEM;
- }
+ dev = init_etherdev(0, sizeof(struct happy_meal));
if (hme_version_printed++ == 0)
printk(KERN_INFO "%s", version);
@@ -2741,7 +2739,8 @@
#endif
#ifdef CONFIG_PCI
-static int __init happy_meal_pci_init(struct net_device *dev, struct pci_dev *pdev)
+static int __devinit happy_meal_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *id)
{
struct quattro *qp = NULL;
#ifdef __sparc__
@@ -2752,6 +2751,10 @@
unsigned long hpreg_base;
int i, qfe_slot = -1;
char prom_name[64];
+ struct net_device *dev;
+
+ if (pci_enable_device(pdev))
+ return -EIO;
/* Now make sure pci_dev cookie is there. */
#ifdef __sparc__
@@ -2778,13 +2781,7 @@
if (qfe_slot == 4)
return -ENODEV;
}
- if (dev == NULL) {
- dev = init_etherdev(0, sizeof(struct happy_meal));
- } else {
- dev->priv = kmalloc(sizeof(struct happy_meal), GFP_KERNEL);
- if (dev->priv == NULL)
- return -ENOMEM;
- }
+ dev = init_etherdev(0, sizeof(struct happy_meal));
if (hme_version_printed++ == 0)
printk(KERN_INFO "%s", version);
@@ -2887,8 +2884,10 @@
/* And of course, indicate this is PCI. */
hp->happy_flags |= HFLAG_PCI;
+#ifdef __sparc__
/* Assume PCI happy meals can handle all burst sizes. */
hp->happy_bursts = DMA_BURSTBITS;
+#endif
hp->happy_block = (struct hmeal_init_block *)
pci_alloc_consistent(pdev, PAGE_SIZE, &hp->hblock_dvma);
@@ -2926,6 +2925,8 @@
hp->write32 = pci_hme_write32;
#endif
+ pdev->driver_data = hp;
+
/* Grrr, Happy Meal comes up by default not advertising
* full duplex 100baseT capabilities, fix this.
*/
@@ -2937,15 +2938,26 @@
* device list.
*/
dev->ifindex = dev_new_index();
- hp->next_module = root_happy_dev;
- root_happy_dev = hp;
return 0;
}
+
+static void __devexit happy_meal_pci_remove (struct pci_dev *pdev)
+{
+ struct happy_meal *hp = pdev->driver_data;
+
+ pci_free_consistent(hp->happy_dev,
+ PAGE_SIZE,
+ hp->happy_block,
+ hp->hblock_dvma);
+ unregister_netdev(hp->dev);
+ kfree(hp->dev);
+}
+
#endif
#ifdef CONFIG_SBUS
-static int __init happy_meal_sbus_probe(struct net_device *dev)
+static int __init happy_meal_sbus_probe(void)
{
struct sbus_bus *sbus;
struct sbus_dev *sdev;
@@ -2959,11 +2971,11 @@
dev = NULL;
if (!strcmp(name, "SUNW,hme")) {
cards++;
- happy_meal_sbus_init(dev, sdev, 0);
+ happy_meal_sbus_init(sdev, 0);
} else if (!strcmp(name, "qfe") ||
!strcmp(name, "SUNW,qfe")) {
cards++;
- happy_meal_sbus_init(dev, sdev, 1);
+ happy_meal_sbus_init(sdev, 1);
}
}
}
@@ -2974,58 +2986,56 @@
#endif
#ifdef CONFIG_PCI
-static int __init happy_meal_pci_probe(struct net_device *dev)
-{
- struct pci_dev *pdev = NULL;
- int cards = 0;
- while ((pdev = pci_find_device(PCI_VENDOR_ID_SUN,
- PCI_DEVICE_ID_SUN_HAPPYMEAL, pdev)) != NULL) {
- if (pci_enable_device(pdev))
- continue;
- if (cards)
- dev = NULL;
- cards++;
- happy_meal_pci_init(dev, pdev);
- }
- return cards;
-}
+static struct pci_device_id happymeal_pci_ids[] __devinitdata = {
+ {
+ vendor: PCI_VENDOR_ID_SUN,
+ device: PCI_DEVICE_ID_SUN_HAPPYMEAL,
+ subvendor: PCI_ANY_ID,
+ subdevice: PCI_ANY_ID,
+ },
+ { } /* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(pci, happymeal_pci_ids);
+
+static struct pci_driver happy_meal_pci_driver = {
+ name: "sunhme",
+ id_table: happymeal_pci_ids,
+ probe: happy_meal_pci_probe,
+ remove: happy_meal_pci_remove,
+};
#endif
static int __init happy_meal_probe(void)
{
- struct net_device *dev = NULL;
static int called = 0;
int cards;
- root_happy_dev = NULL;
-
if (called)
return -ENODEV;
called++;
cards = 0;
#ifdef CONFIG_SBUS
- cards += happy_meal_sbus_probe(dev);
- if (cards != 0)
- dev = NULL;
+ root_happy_dev = NULL;
+ cards += happy_meal_sbus_probe();
#endif
#ifdef CONFIG_PCI
- cards += happy_meal_pci_probe(dev);
+ return pci_module_init(&happy_meal_pci_driver);
+#else
+ return (cards > 0) ? 0 : -ENODEV;
#endif
- if (!cards)
- return -ENODEV;
- return 0;
}
static void __exit happy_meal_cleanup_module(void)
{
+#ifdef CONFIG_SBUS
while (root_happy_dev) {
struct happy_meal *hp = root_happy_dev;
struct happy_meal *next = root_happy_dev->next_module;
-#ifdef CONFIG_SBUS
if (!(hp->happy_flags & HFLAG_PCI)) {
sbus_iounmap(hp->gregs, GREG_REG_SIZE);
sbus_iounmap(hp->etxregs, ETX_REG_SIZE);
@@ -3037,19 +3047,14 @@
hp->happy_block,
hp->hblock_dvma);
}
-#endif
-#ifdef CONFIG_PCI
- if ((hp->happy_flags & HFLAG_PCI)) {
- pci_free_consistent(hp->happy_dev,
- PAGE_SIZE,
- hp->happy_block,
- hp->hblock_dvma);
- }
-#endif
unregister_netdev(hp->dev);
kfree(hp->dev);
root_happy_dev = next;
}
+#endif
+#ifdef CONFIG_PCI
+ pci_unregister_driver (&happy_meal_pci_driver);
+#endif
}
module_init(happy_meal_probe);
I never ported it to the new PCI interfaces strictly because when
combined with SBUS it makes the driver initialization look really
sloppy.
Sorry, I don't like this change.
Later,
David S. Miller
[email protected]
>Sorry, I don't like this change.
Can you at least add the MODULE_DEVICE_TABLE declaration
and the pci_device_id table that it refers to, even if the code
does not directly reference it? (You can make it as __initdata
rather than __devinitdata, since it can safely be thrown away.)
That was automatic PCI ID recognition will work.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
From: "Adam J. Richter" <[email protected]>
Date: Thu, 16 Nov 2000 03:10:14 -0800
>Sorry, I don't like this change.
Can you at least add the MODULE_DEVICE_TABLE declaration
and the pci_device_id table that it refers to, even if the code
does not directly reference it? (You can make it as __initdata
rather than __devinitdata, since it can safely be thrown away.)
That was automatic PCI ID recognition will work.
Feel free to send me a patch which does this. No problem.
Later,
David S. Miller
[email protected]
In article <[email protected]> you wrote:
> I never ported it to the new PCI interfaces strictly because when
> combined with SBUS it makes the driver initialization look really
> sloppy.
BTW, what do you think of a new PCI style probing for SBUS?
When I hacked on a small sbus driver, I thought it might be a good idea
to have tables like the new PCI interface for all busses.
The interface for SBUS would be:
struct sbus_device_id {
char * promname;
unsigned long driver_data;
};
struct sbus_driver {
struct list_head node;
char * name;
struct sbus_device_id * id_table;
int (* probe)(struct sbus_dev * dev, const struct sbus_device_id * id);
void (* remove)(struct sbus_dev * dev);
};
Would you accept such a change for 2.5?
Christoph
--
Always remember that you are unique. Just like everyone else.
Date: Thu, 16 Nov 2000 13:13:37 +0100
From: Christoph Hellwig <[email protected]>
Would you accept such a change for 2.5?
Sure, that sounds nice.
Actually, one of the possible "grand plans" for 2.5 is a unified
"struct device". I don't know what will actually happen here.
Later,
David S. Miller
[email protected]
On Thu, Nov 16, 2000 at 04:22:36AM -0800, David S. Miller wrote:
> Sure, that sounds nice.
>
> Actually, one of the possible "grand plans" for 2.5 is a unified
> "struct device". I don't know what will actually happen here.
apologies for pointing out the potentially obvoius, but people might
want to investigate 'newbus' while brainstorming about this:
http://www.daemonnews.org/200007/newbus-intro.html
--
zach
"Adam J. Richter" wrote:
> -static struct happy_meal *root_happy_dev = NULL;
> -
> #ifdef CONFIG_SBUS
> +static struct happy_meal *root_happy_dev = NULL;
> static struct quattro *qfe_sbus_list = NULL;
> #endif
don't initialize static to zero/null explicitly..
> - if (dev == NULL) {
> - dev = init_etherdev(0, sizeof(struct happy_meal));
> - } else {
> - dev->priv = kmalloc(sizeof(struct happy_meal), GFP_KERNEL);
> - if (dev->priv == NULL)
> - return -ENOMEM;
> - }
> + dev = init_etherdev(0, sizeof(struct happy_meal));
allocation failure not checked
> -static int __init happy_meal_pci_init(struct net_device *dev, struct pci_dev *pdev)
> +static int __devinit happy_meal_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
Are you aware of any hotplug sunhme hardware? If no, don't change it to
__devinit...
> - if (dev == NULL) {
> - dev = init_etherdev(0, sizeof(struct happy_meal));
> - } else {
> - dev->priv = kmalloc(sizeof(struct happy_meal), GFP_KERNEL);
> - if (dev->priv == NULL)
> - return -ENOMEM;
> - }
> + dev = init_etherdev(0, sizeof(struct happy_meal));
check for failure.
also, ether_setup() call is not needed if you always call
init_etherdev(NULL, ...).
> +static void __devexit happy_meal_pci_remove (struct pci_dev *pdev)
> +{
> + struct happy_meal *hp = pdev->driver_data;
> +
> + pci_free_consistent(hp->happy_dev,
> + PAGE_SIZE,
> + hp->happy_block,
> + hp->hblock_dvma);
> + unregister_netdev(hp->dev);
> + kfree(hp->dev);
> +}
zero pdev->driver_data. If this driver has to be portable, use
pci_{get,set}_drvdata() instead of directly accessing ::driver_data.
> +static struct pci_device_id happymeal_pci_ids[] __devinitdata = {
again, no __devxxx if not hotplug.
> #ifdef CONFIG_PCI
> - cards += happy_meal_pci_probe(dev);
> + return pci_module_init(&happy_meal_pci_driver);
> +#else
> + return (cards > 0) ? 0 : -ENODEV;
> #endif
ifdef not needed
> +#ifdef CONFIG_PCI
> + pci_unregister_driver (&happy_meal_pci_driver);
> +#endif
ifdef not needed.
--
Jeff Garzik |
Building 1024 | The chief enemy of creativity is "good" sense
MandrakeSoft | -- Picasso
Jeff Garzik writes:
>Are you aware of any hotplug sunhme hardware? If no, don't change it to
>__devinit...
Can I have a hot plug PCI bridge card that connects to
a regular PCI backplane (perhaps as some kind of CardBus docking
station card)? If so, all PCI drivers should use __dev{init,exit}{,data}.
The other excellent points that you raise apply equally to
the driver before and after my patch (although my patch made it
more clear that the struct netdevice parameters previously being
passed around were always null). It is important to realize this
becase, as of yesterday, Dave had said that he was not going to
apply the new style PCI changes at this point, but had integrated
the MODULE_DEVICE_TABLE changes. So, Dave: you should look at
the points that Jeff raised, even if you are not integrating
the rest of my new style PCI patch.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
"Adam J. Richter" wrote:
> Jeff Garzik writes:
> >Are you aware of any hotplug sunhme hardware? If no, don't change it to
> >__devinit...
>
> Can I have a hot plug PCI bridge card that connects to
> a regular PCI backplane (perhaps as some kind of CardBus docking
> station card)? If so, all PCI drivers should use __dev{init,exit}{,data}.
I am willing to consider adding __devxxx only when other __devxxx
entries already exist.
These conversions to _devxxx are too late in the freeze, and only have
value for isolated cases --which you admit you don't even know exist--.
Linus Rule 1: Don't overdesign. Even if such cases do exist, and this
is a need, it should be addressed some other way.
Your suggestion bloats drivers needlessly for the majority of cases and
I will not be applying any such patches...
Jeff
--
Jeff Garzik |
Building 1024 | The chief enemy of creativity is "good" sense
MandrakeSoft | -- Picasso
>I am willing to consider adding __devxxx only when other __devxxx
>entries already exist.
>These conversions to _devxxx are too late in the freeze, and only have
>value for isolated cases --which you admit you don't even know exist--.
>Linus Rule 1: Don't overdesign.
Even ignoring CardBus, there apparently are docking
stations that have PCI busses, such as shown in
http://www.pangolin.com/LD2000/dock/gateway.htm ("Each supports hot
docking, so you can attach or detach your system without turning it
off"), and a web search turns up plenty of hits, at least for mobile
chipsets apparently designed for this. It looks like this is a
pretty standard capability.
Even if we were unsure, the more conservative approach would
be to use __devinitdata. The only cost of incorrectly using __devinitdata
instead of __initdata, would be to make the effected kernel module
use ~100 bytes more unswappable kernel memory when CONFIG_HOTPLUG is
defined and the module is loaded (28 bytes per entry, including a null
entry). On the other side that risk comparison, the cost of incorrectly
using __initdata when __devinitdata was correct is that the user's
KERNEL WILL CRASH when the notebook is inserted or removed from such a
docking station, even when the kernel is built with CONFIG_HOTPLUG.
Although I'm not into quoting any programmer like some religious
figure, I will say that I think you're misinterpreting the meaning of
an admonition like "Don't overdesign." We are not talking about designing
some new abstraction or making the code more complex, but rather
selecting rather a choice between two words, one of which is the more
conservatively crash resistant __devinitdata, the other of which would
save 28 bytes in CONFIG_HOTPLUG kernels only, and at the expense of
probably causing kernel crashes.
>Even if such cases do exist, and this
>is a need, it should be addressed some other way.
1. Why?
2. What other way did you have in mind?
By the way, although I do not have the PCI standard here, I do
see from _PCI System Architecture_, 4th edition, chapter 19,
"Configuration Registers", in the "Device ID Register" section that
devices "designed around the same third party core logic" are allowed
to have the same device ID (and they are even complaint to PCI 2.2 if
they have different Subsystem ID values). So, my approach also has
the minor advantage that if a vendor wants to ship a hot pluggable
version of their PCI card in the future with the same device ID
(a likely decision), then there may not need to coordinate a new
release of the Linux driver. (This is a really small benefit; the
kernel crashes that you want to change my existing patches to produce
is the big issue.)
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
I wrote:
>[...] the cost of incorrectly
>using __initdata when __devinitdata was correct is that the user's
>KERNEL WILL CRASH when the notebook is inserted or removed from such a
>docking station, even when the kernel is built with CONFIG_HOTPLUG.
My statement above, without some missing qualification, is wrong.
I should have qualified this statement more carefully. (I'm sure the
flames are already in the mail about this.) The kernel will crash in
any case if the relevant driver does not support hot plugging.and the
notebook is being removed with the PCI driver still loaded.
For drivers that do not support hot plugging, we could use
__initdata instead of __devinitdata, since they will crash in any
case. However, violating the instructions in Documentation/pci.txt
("The ID table array should be marked __devinitdata") in this way
will provoke a slew of driver bugs as the over one hundred remaining
PCI drivers are converted to the new PCI interface and some authors
overlook the need to change the MODULE_DEVICE_TABLE storage class.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."
Adam J. Richter writes:
> Can I have a hot plug PCI bridge card that connects to
> a regular PCI backplane (perhaps as some kind of CardBus docking
> station card)? If so, all PCI drivers should use __dev{init,exit}{,data}.
PCI is certainly hot-plug hardware, but not on common desktop PCs.
Since PCI is so popular and so often not hot-plug, users should
not be forced to have hot-plug PCI support when they only need
hot-plug SCSI, etc.
Obvious hack: __pciinit, __pciexit, __pciinitdata...
Albert D. Cahalan <[email protected]> writes:
>PCI is certainly hot-plug hardware, but not on common desktop PCs.
>Since PCI is so popular and so often not hot-plug, users should
>not be forced to have hot-plug PCI support when they only need
>hot-plug SCSI, etc.
>Obvious hack: __pciinit, __pciexit, __pciinitdata...
Yes, as I mentioned in a previous discussion, sometime after
2.4.0, I would like to see CONFIG_HOTPLUG replaced with CONFIG_PCI_HOTPLUG
with __pci{init,exit}{,data}, and CONFIG_USB_HOTPLUG with
__usb{init,exit}{,data} and likewise for other busses, since these
facilities are completely independent, and there are reasons for
wanting to compile in one facility compiled in and not the others,
and it would make drivers self-document which hotplug facility
is the reason why something should be marked as __dev{init,exit}{,data}.
Adam J. Richter __ ______________ 4880 Stevens Creek Blvd, Suite 104
[email protected] \ / San Jose, California 95129-1034
+1 408 261-6630 | g g d r a s i l United States of America
fax +1 408 261-6631 "Free Software For The Rest Of Us."