| Message ID | 49E7239B.4000309@myri.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Brice Goglin <brice@myri.com> Date: Thu, 16 Apr 2009 14:24:59 +0200 > Add myri10ge_fw_names to override the default firmware > on a per-board basis. > > Signed-off-by; Brice Goglin <brice@myri.com> ^ semi-colon should be colon Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 16 Apr 2009 14:24:59 +0200 Brice Goglin <brice@myri.com> wrote: > --- linux-tmp.old/drivers/net/myri10ge/myri10ge.c 2009-04-08 06:56:50.000000000 +0200 > +++ linux-tmp/drivers/net/myri10ge/myri10ge.c 2009-04-09 14:00:16.000000000 +0200 > @@ -255,6 +255,7 @@ > u32 read_write_dma; > u32 link_changes; > u32 msg_enable; > + unsigned int board_number; > }; > > static char *myri10ge_fw_unaligned = "myri10ge_ethp_z8e.dat"; > @@ -266,6 +267,13 @@ > module_param(myri10ge_fw_name, charp, S_IRUGO | S_IWUSR); > MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image name"); > > +#define MYRI10GE_MAX_BOARDS 8 > +static char *myri10ge_fw_names[MYRI10GE_MAX_BOARDS] = > + {[0...(MYRI10GE_MAX_BOARDS - 1)] = NULL }; > +module_param_array_named(myri10ge_fw_names, myri10ge_fw_names, charp, NULL, > + 0444); > +MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image names per board"); > + > static int myri10ge_ecrc_enable = 1; > module_param(myri10ge_ecrc_enable, int, S_IRUGO); > MODULE_PARM_DESC(myri10ge_ecrc_enable, "Enable Extended CRC on PCI-E"); > @@ -3258,6 +3266,8 @@ > > static void myri10ge_select_firmware(struct myri10ge_priv *mgp) > { > + int overridden = 0; > + > if (myri10ge_force_firmware == 0) { > int link_width, exp_cap; > u16 lnk; > @@ -3291,10 +3301,18 @@ > } > } > if (myri10ge_fw_name != NULL) { > - dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", > - myri10ge_fw_name); > + overridden = 1; > mgp->fw_name = myri10ge_fw_name; > } > + if (mgp->board_number < MYRI10GE_MAX_BOARDS && > + myri10ge_fw_names[mgp->board_number] != NULL && > + strlen(myri10ge_fw_names[mgp->board_number])) { > + mgp->fw_name = myri10ge_fw_names[mgp->board_number]; > + overridden = 1; > + } > + if (overridden) > + dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", > + mgp->fw_name); > } > > #ifdef CONFIG_PM > @@ -3759,6 +3777,7 @@ > int status = -ENXIO; > int dac_enabled; > unsigned hdr_offset, ss_offset; > + static int board_number; > > netdev = alloc_etherdev_mq(sizeof(*mgp), MYRI10GE_MAX_SLICES); > if (netdev == NULL) { > @@ -3775,6 +3794,7 @@ > mgp->pause = myri10ge_flow_control; > mgp->intr_coal_delay = myri10ge_intr_coal_delay; > mgp->msg_enable = netif_msg_init(myri10ge_debug, MYRI10GE_MSG_DEFAULT); > + mgp->board_number = board_number; > init_waitqueue_head(&mgp->down_wq); > > if (pci_enable_device(pdev)) { > @@ -3925,6 +3945,7 @@ > netdev->irq, mgp->tx_boundary, mgp->fw_name, > (mgp->wc_enabled ? "Enabled" : "Disabled")); > > + board_number++; > return 0; > > abort_with_state: Code assumes boards are always initialized in the same order. I suppose this is always true and is nothing wrong here. However perhaps linux could provide some form of modules/boot parameters that are used with an identification number like MAC address. Example: module my_parms="id1:param1;id2:param2;" char *get_module_param_id(const char *param, const char *id) This would help in situation like this - admin will not have to thinking about boards initialization ordering. Besides some other drivers (like MTD cmd partitions) use own parsing for similar things, I think would be nice to unify things. What You think? Cheers Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stanislaw Gruszka wrote: > On Thu, 16 Apr 2009 14:24:59 +0200 > Brice Goglin <brice@myri.com> wrote: > > >> --- linux-tmp.old/drivers/net/myri10ge/myri10ge.c 2009-04-08 06:56:50.000000000 +0200 >> +++ linux-tmp/drivers/net/myri10ge/myri10ge.c 2009-04-09 14:00:16.000000000 +0200 >> @@ -255,6 +255,7 @@ >> u32 read_write_dma; >> u32 link_changes; >> u32 msg_enable; >> + unsigned int board_number; >> }; >> >> static char *myri10ge_fw_unaligned = "myri10ge_ethp_z8e.dat"; >> @@ -266,6 +267,13 @@ >> module_param(myri10ge_fw_name, charp, S_IRUGO | S_IWUSR); >> MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image name"); >> >> +#define MYRI10GE_MAX_BOARDS 8 >> +static char *myri10ge_fw_names[MYRI10GE_MAX_BOARDS] = >> + {[0...(MYRI10GE_MAX_BOARDS - 1)] = NULL }; >> +module_param_array_named(myri10ge_fw_names, myri10ge_fw_names, charp, NULL, >> + 0444); >> +MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image names per board"); >> + >> static int myri10ge_ecrc_enable = 1; >> module_param(myri10ge_ecrc_enable, int, S_IRUGO); >> MODULE_PARM_DESC(myri10ge_ecrc_enable, "Enable Extended CRC on PCI-E"); >> @@ -3258,6 +3266,8 @@ >> >> static void myri10ge_select_firmware(struct myri10ge_priv *mgp) >> { >> + int overridden = 0; >> + >> if (myri10ge_force_firmware == 0) { >> int link_width, exp_cap; >> u16 lnk; >> @@ -3291,10 +3301,18 @@ >> } >> } >> if (myri10ge_fw_name != NULL) { >> - dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", >> - myri10ge_fw_name); >> + overridden = 1; >> mgp->fw_name = myri10ge_fw_name; >> } >> + if (mgp->board_number < MYRI10GE_MAX_BOARDS && >> + myri10ge_fw_names[mgp->board_number] != NULL && >> + strlen(myri10ge_fw_names[mgp->board_number])) { >> + mgp->fw_name = myri10ge_fw_names[mgp->board_number]; >> + overridden = 1; >> + } >> + if (overridden) >> + dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", >> + mgp->fw_name); >> } >> >> #ifdef CONFIG_PM >> @@ -3759,6 +3777,7 @@ >> int status = -ENXIO; >> int dac_enabled; >> unsigned hdr_offset, ss_offset; >> + static int board_number; >> >> netdev = alloc_etherdev_mq(sizeof(*mgp), MYRI10GE_MAX_SLICES); >> if (netdev == NULL) { >> @@ -3775,6 +3794,7 @@ >> mgp->pause = myri10ge_flow_control; >> mgp->intr_coal_delay = myri10ge_intr_coal_delay; >> mgp->msg_enable = netif_msg_init(myri10ge_debug, MYRI10GE_MSG_DEFAULT); >> + mgp->board_number = board_number; >> init_waitqueue_head(&mgp->down_wq); >> >> if (pci_enable_device(pdev)) { >> @@ -3925,6 +3945,7 @@ >> netdev->irq, mgp->tx_boundary, mgp->fw_name, >> (mgp->wc_enabled ? "Enabled" : "Disabled")); >> >> + board_number++; >> return 0; >> >> abort_with_state: >> > > Code assumes boards are always initialized in the same order. I suppose > this is always true and is nothing wrong here. However perhaps linux could > provide some form of modules/boot parameters that are used with > an identification number like MAC address. Example: > > module my_parms="id1:param1;id2:param2;" > > char *get_module_param_id(const char *param, const char *id) > > This would help in situation like this - admin will not have to thinking > about boards initialization ordering. Besides some other drivers (like MTD > cmd partitions) use own parsing for similar things, I think would be > nice to unify things. What You think? > We actually thought about supporting "eth2:fwname1,eth0:fwname2". But it might be hard to implement in this case due to udev possible renaming interfaces and this firmware names being needed *before* the renaming. So yes, an automatic way to handle such parameter strings might be good, but maybe not in this exact case. Brice -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-04-18 at 20:36 +0200, Brice Goglin wrote: > Stanislaw Gruszka wrote: > > > This would help in situation like this - admin will not have to thinking > > about boards initialization ordering. Besides some other drivers (like MTD > > cmd partitions) use own parsing for similar things, I think would be > > nice to unify things. What You think? > > > > We actually thought about supporting "eth2:fwname1,eth0:fwname2". But it > might be hard to implement in this case due to udev possible renaming > interfaces and this firmware names being needed *before* the renaming. > So yes, an automatic way to handle such parameter strings might be good, > but maybe not in this exact case. It seems like this could be done in user space, using the PCI bus ID as a key to select the firmware. The uevent identifies which device is requesting the firmware, so some modification to /lib/udev/firmware.sh should do it. Not that I really want to encourage relying on the bus ID being part of the path to the firmware files, as if no one uses it, it makes it easier to cache firmware files for multiple devices, but that's work for another day and the ABI is already set anyway... Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-04-18 at 20:56 -0400, David Dillow wrote: > On Sat, 2009-04-18 at 20:36 +0200, Brice Goglin wrote: > > Stanislaw Gruszka wrote: > > > > > This would help in situation like this - admin will not have to thinking > > > about boards initialization ordering. Besides some other drivers (like MTD > > > cmd partitions) use own parsing for similar things, I think would be > > > nice to unify things. What You think? > > > > > > > We actually thought about supporting "eth2:fwname1,eth0:fwname2". But it > > might be hard to implement in this case due to udev possible renaming > > interfaces and this firmware names being needed *before* the renaming. > > So yes, an automatic way to handle such parameter strings might be good, > > but maybe not in this exact case. > > It seems like this could be done in user space, using the PCI bus ID as > a key to select the firmware. The uevent identifies which device is > requesting the firmware, so some modification to /lib/udev/firmware.sh > should do it. On further inspection, that should work on Fedora 10, and it looks like other distros that use the upstream udev rules. RHEL5 uses a binary helper, so changing 05-early-rules.sh to use a script similar to udev's firmware.sh would work. YMMV elsewhere, those are the ones I had handy and happened to be working on some udev rules. Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Dillow <dave@thedillows.org> Date: Sat, 18 Apr 2009 20:56:57 -0400 > It seems like this could be done in user space, using the PCI bus ID as > a key to select the firmware. PCI BUS ID's are not in any way unique, you would need to include the PCI Domain as well. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-04-18 at 21:09 -0700, David Miller wrote: > From: David Dillow <dave@thedillows.org> > Date: Sat, 18 Apr 2009 20:56:57 -0400 > > > It seems like this could be done in user space, using the PCI bus ID as > > a key to select the firmware. > > PCI BUS ID's are not in any way unique, you would need to include the > PCI Domain as well. I had that in mind, but I wasn't precise in my wording. I think that info is there as well. IIRC, and brief glance at the code suggests, that the uevent for the firmware requests gets the environment variable DEVPATH that would contains that info: DEVPATH=/devices/pci0000:00/0000:00:1e.0/0000:09:01.0 Now I'm curious, so I'll probably re-verify that with udevmonitor, a recent kernel and the typhoon driver, since it is the only thing I have that requests firmware. But sometime after I sleep... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 18, 2009 at 8:56 PM, David Dillow <dave@thedillows.org> wrote: > On Sat, 2009-04-18 at 20:36 +0200, Brice Goglin wrote: >> Stanislaw Gruszka wrote: >> >> > This would help in situation like this - admin will not have to thinking >> > about boards initialization ordering. Besides some other drivers (like MTD >> > cmd partitions) use own parsing for similar things, I think would be >> > nice to unify things. What You think? >> > >> >> We actually thought about supporting "eth2:fwname1,eth0:fwname2". But it >> might be hard to implement in this case due to udev possible renaming >> interfaces and this firmware names being needed *before* the renaming. >> So yes, an automatic way to handle such parameter strings might be good, >> but maybe not in this exact case. > > It seems like this could be done in user space, using the PCI bus ID as > a key to select the firmware. The uevent identifies which device is > requesting the firmware, so some modification to /lib/udev/firmware.sh > should do it. > > Not that I really want to encourage relying on the bus ID being part of > the path to the firmware files, as if no one uses it, it makes it easier > to cache firmware files for multiple devices, but that's work for > another day and the ABI is already set anyway... The company I work for (eXMeritus) actually had an internal patch to do something like this on a much older version of the driver (and I believe we may have been the original motivation for this new patch). We were fiddling with the firmware path to first look up (for example) "myri10ge_eth_z8e_${PCI_BUS_ADDR}.dat", followed by "myri10ge_eth_z8e.dat" if that failed. Unfortunately on the RHEL5 kernel we were using at the time, the firmware filenames seemed to get truncated at 29 characters or something, because we got requests for "/lib/firmware/myri10ge_eth_z8e_0000:08:0a.0" and "/lib/firmware/myri10ge_ethp_z8e_0000:08:0a." As David notes, RHEL5 uses its own binary helper that would need some extra help (which was why we wrote our original patch), but we would be perfectly fine with doing normal udev rule fiddling on any other Linux distro. Cheers, Kyle Moffett -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 18 Apr 2009 22:36:39 -0400 David Dillow <dave@thedillows.org> wrote: > > > We actually thought about supporting "eth2:fwname1,eth0:fwname2". But it > > > might be hard to implement in this case due to udev possible renaming > > > interfaces and this firmware names being needed *before* the renaming. I was thinking about ID as serial number or MAC address - rather than device name - something that driver specific and uniquely identify device. > > It seems like this could be done in user space, using the PCI bus ID as > > a key to select the firmware. The uevent identifies which device is > > requesting the firmware, so some modification to /lib/udev/firmware.sh > > should do it. > > On further inspection, that should work on Fedora 10, and it looks like > other distros that use the upstream udev rules. RHEL5 uses a binary > helper, so changing 05-early-rules.sh to use a script similar to udev's > firmware.sh would work. YMMV elsewhere, those are the ones I had handy > and happened to be working on some udev rules. Module parameters I'm thinking about are not intended only for firmware names, it could replace any device parameter which is needed during initialization. Currently devices use module_param_array and code like this: dev->my_param = default; if (board_no < MAX_BOARDS) dev->my_param = my_param[board_no++]; This solution have drawbacks: - it limits number of device instances which can get parameter - assume initialization ordering is constant. - assume initialization is not done in parallel Providing module_param_id would address this issues, but I have some concerns before doing it: - device have to access ID (like serial no) before initialization (at least before driver will need parameter) - why nobody already done it - perhaps such thing would be totally useless Cheers Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stanislaw Gruszka wrote: > Providing module_param_id would address this issues, but I have some concerns > before doing it: > > - device have to access ID (like serial no) before initialization (at least > before driver will need parameter) > - why nobody already done it - perhaps such thing would be totally useless > Per-nic module parameters are used in some Intel NIC drivers but it's not considered a very good solution in the general case: http://kerneltrap.org/index.php?q=mailarchive/linux-netdev/2008/10/24/3801824/thread But again, when talking about changing the NIC firmware, ethtool and friends are not that easy. Brice -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Apr 2009 11:55:15 +0200 Brice Goglin <brice@myri.com> wrote: > Stanislaw Gruszka wrote: > > Providing module_param_id would address this issues, but I have some concerns > > before doing it: > > > > - device have to access ID (like serial no) before initialization (at least > > before driver will need parameter) > > - why nobody already done it - perhaps such thing would be totally useless > > > > Per-nic module parameters are used in some Intel NIC drivers but it's > not considered a very good solution in the general case: > > http://kerneltrap.org/index.php?q=mailarchive/linux-netdev/2008/10/24/3801824/thread > > But again, when talking about changing the NIC firmware, ethtool and > friends are not that easy. I agree that device parameters, which are not needed during initialization or can be changed at runtime shall not be module parameters. I agree also that some parameters are needed during startup when driver loads and are driver specific. For these modules parameters shall be used. Firmware is one example, I think we have others. Cheers Stanislaw -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-tmp.old/drivers/net/myri10ge/myri10ge.c 2009-04-08 06:56:50.000000000 +0200 +++ linux-tmp/drivers/net/myri10ge/myri10ge.c 2009-04-09 14:00:16.000000000 +0200 @@ -255,6 +255,7 @@ u32 read_write_dma; u32 link_changes; u32 msg_enable; + unsigned int board_number; }; static char *myri10ge_fw_unaligned = "myri10ge_ethp_z8e.dat"; @@ -266,6 +267,13 @@ module_param(myri10ge_fw_name, charp, S_IRUGO | S_IWUSR); MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image name"); +#define MYRI10GE_MAX_BOARDS 8 +static char *myri10ge_fw_names[MYRI10GE_MAX_BOARDS] = + {[0...(MYRI10GE_MAX_BOARDS - 1)] = NULL }; +module_param_array_named(myri10ge_fw_names, myri10ge_fw_names, charp, NULL, + 0444); +MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image names per board"); + static int myri10ge_ecrc_enable = 1; module_param(myri10ge_ecrc_enable, int, S_IRUGO); MODULE_PARM_DESC(myri10ge_ecrc_enable, "Enable Extended CRC on PCI-E"); @@ -3258,6 +3266,8 @@ static void myri10ge_select_firmware(struct myri10ge_priv *mgp) { + int overridden = 0; + if (myri10ge_force_firmware == 0) { int link_width, exp_cap; u16 lnk; @@ -3291,10 +3301,18 @@ } } if (myri10ge_fw_name != NULL) { - dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", - myri10ge_fw_name); + overridden = 1; mgp->fw_name = myri10ge_fw_name; } + if (mgp->board_number < MYRI10GE_MAX_BOARDS && + myri10ge_fw_names[mgp->board_number] != NULL && + strlen(myri10ge_fw_names[mgp->board_number])) { + mgp->fw_name = myri10ge_fw_names[mgp->board_number]; + overridden = 1; + } + if (overridden) + dev_info(&mgp->pdev->dev, "overriding firmware to %s\n", + mgp->fw_name); } #ifdef CONFIG_PM @@ -3759,6 +3777,7 @@ int status = -ENXIO; int dac_enabled; unsigned hdr_offset, ss_offset; + static int board_number; netdev = alloc_etherdev_mq(sizeof(*mgp), MYRI10GE_MAX_SLICES); if (netdev == NULL) { @@ -3775,6 +3794,7 @@ mgp->pause = myri10ge_flow_control; mgp->intr_coal_delay = myri10ge_intr_coal_delay; mgp->msg_enable = netif_msg_init(myri10ge_debug, MYRI10GE_MSG_DEFAULT); + mgp->board_number = board_number; init_waitqueue_head(&mgp->down_wq); if (pci_enable_device(pdev)) { @@ -3925,6 +3945,7 @@ netdev->irq, mgp->tx_boundary, mgp->fw_name, (mgp->wc_enabled ? "Enabled" : "Disabled")); + board_number++; return 0; abort_with_state: