Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Stanislaw Gruszka]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Brice Goglin]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[David Dillow]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[David Dillow]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Stanislaw Gruszka]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Brice Goglin]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Stanislaw Gruszka]

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

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[David Miller]

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.

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[David Dillow]

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...

Re: [PATCH net-next] myri10ge: allow per-board firmware overriding

[Kyle Moffett]

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