This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
address in Open Firmware or IDPROM").
As with that fix, attempt to look up the MAC address in Open Firmware
on systems that support it, and use IDPROM on SPARC if no OF address
is found.
Reviewed-by: Martin K. Petersen <redacted>
Signed-off-by: Sowmini Varadhan <redacted>
---
v2: andy shevchenko comments
v3: more andy shevchenko comments
drivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++
1 files changed, 30 insertions(+), 0 deletions(-)
From: Andy Shevchenko <hidden> Date: 2015-10-30 18:06:34
On Fri, Oct 30, 2015 at 5:03 PM, Sowmini Varadhan
[off-list ref] wrote:
This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
address in Open Firmware or IDPROM").
As with that fix, attempt to look up the MAC address in Open Firmware
on systems that support it, and use IDPROM on SPARC if no OF address
is found.
Reviewed-by: Martin K. Petersen <redacted>
Signed-off-by: Sowmini Varadhan <redacted>
Few nitpicks below, otherwise:
Reviewed-by: Andy Shevchenko <redacted>
quoted hunk
---
v2: andy shevchenko comments
v3: more andy shevchenko comments
drivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++
1 files changed, 30 insertions(+), 0 deletions(-)
Without it, you will get:
:
CC [M] drivers/net/ethernet/intel/i40e/i40e_common.o
drivers/net/ethernet/intel/i40e/i40e_common.c: In function ?i40e_get_platform_mac_addr?:
drivers/net/ethernet/intel/i40e/i40e_common.c:1021: error: dereferencing pointer to incomplete type
Unless you feel passionately about the \n nits, I'm going to pass on those.
Thanks for reviewing.
--Sowmini
Without it, you will get:
:
CC [M] drivers/net/ethernet/intel/i40e/i40e_common.o
drivers/net/ethernet/intel/i40e/i40e_common.c: In function ?i40e_get_platform_mac_addr?:
drivers/net/ethernet/intel/i40e/i40e_common.c:1021: error: dereferencing pointer to incomplete type
OK
Unless you feel passionately about the \n nits, I'm going to pass on those.
Thanks for reviewing.
I'm okay, whatever maintainer now accepts.
--
With Best Regards,
Andy Shevchenko
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: Friday, October 30, 2015 8:04 AM
To:
Subject: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM
This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
address in Open Firmware or IDPROM").
As with that fix, attempt to look up the MAC address in Open Firmware
on systems that support it, and use IDPROM on SPARC if no OF address
is found.
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in i40e_main.c, rather than in i40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay in i40e_main.c. That would also take care of one of the include file nits that Andy mentioned.
At the risk of flying my Device Tree ignorance for all to see, I have a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it get into the device tree? I thought the device tree was compiled ahead of time for the platform it is used on. Is this a generic DT pattern just in case some platform actually has the mac-address? Or does the device mac-address get saved into the DT on the first time through this path so it can be found next time?
If the Device Tree has a different mac address than the HW, when does the kernel tell the HW to use a different mac address? Does the DT management eventually call the ndo_set_mac_address call so the HW knows to use a different mac address?
Thanks,
sln
quoted hunk
Reviewed-by: Martin K. Petersen <redacted>
Signed-off-by: Sowmini Varadhan <redacted>
---
v2: andy shevchenko comments
v3: more andy shevchenko comments
drivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++
1 files changed, 30 insertions(+), 0 deletions(-)
Going along with this being the equivalent of the ixgbe patch, I'd
prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
In the design of our drivers, the common file is essentially a device
specific layer, and the OS and platform related stuff should stay in
i40e_main.c. That would also take care of one of the include file nits
that Andy mentioned.
ok, and that was my initial instinct as well, except that I noticed
that the existing i40e code tries interesting variations from the
typical intel driver model by calling i40e_get_mac_addr() which lives in
(for reasons not obvious to me) i40e_common.c
So I assumed there must be some other deeper wisdom at work here.
At the risk of flying my Device Tree ignorance for all to see, I have
a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it
get into the device tree? I thought the device tree was compiled ahead
of time for the platform it is used on. Is this a generic DT pattern
just in case some platform actually has the mac-address? Or does the
device mac-address get saved into the DT on the first time through this
path so it can be found next time?
If the Device Tree has a different mac address than the HW, when
does the kernel tell the HW to use a different mac address? Does the
DT management eventually call the ndo_set_mac_address call so the HW
knows to use a different mac address?
yes, and here I was hoping for some feedback from the intel folks as
well. Commit c762dff24c06 sets hw->mac.perm_addr. I dont know
if there is some similar i40e state that needs to be set.
Please share insights.
--Sowmini
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: Friday, October 30, 2015 11:37 AM
To:
Subject: Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or
IDPROM
On (10/30/15 18:28), Nelson, Shannon wrote:
quoted
Going along with this being the equivalent of the ixgbe patch, I'd
prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
In the design of our drivers, the common file is essentially a device
specific layer, and the OS and platform related stuff should stay in
i40e_main.c. That would also take care of one of the include file nits
that Andy mentioned.
ok, and that was my initial instinct as well, except that I noticed
that the existing i40e code tries interesting variations from the
typical intel driver model by calling i40e_get_mac_addr() which lives in
(for reasons not obvious to me) i40e_common.c
So I assumed there must be some other deeper wisdom at work here.
I'm not sure about any deeper wisdom, but the HW/FW model that this device uses is rather different from our previous devices, so our SW abstractions are a little different from ixgbe and igb.
quoted
At the risk of flying my Device Tree ignorance for all to see, I have
a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it
get into the device tree? I thought the device tree was compiled ahead
of time for the platform it is used on. Is this a generic DT pattern
just in case some platform actually has the mac-address? Or does the
device mac-address get saved into the DT on the first time through this
path so it can be found next time?
If the Device Tree has a different mac address than the HW, when
does the kernel tell the HW to use a different mac address? Does the
DT management eventually call the ndo_set_mac_address call so the HW
knows to use a different mac address?
yes, and here I was hoping for some feedback from the intel folks as
well. Commit c762dff24c06 sets hw->mac.perm_addr. I dont know
if there is some similar i40e state that needs to be set.
Please share insights.
--Sowmini
Forgive me if you're already up with this, I just want to be sure we're thinking along the same lines.
The HW has a mac address burned into its own eeprom which it will use by default. The driver merely queries the HW to find out what that mac address is and report it to the OS so it can be registered correctly in the netdev.
If a different mac address is desired, perhaps from the user issuing an "ip link" or ifconfig command, the OS will call the netdev's ndo_set_mac_address to tell the driver. In our case, the driver then tells the HW device to filter for the new mac address rather than the eeprom'd address. This remains until a reset when the HW goes back to its eeprom'd address.
In the cases in which I'm aware, the platform manufacturer normally will burn a different mac-address into the device's eeprom at manufacturing time if they want something other than what came from Intel. I suppose a Device Tree oriented platform could have a different address in the DT, but somehow that needs to get communicated to the device driver so that it can tell the HW.
My question to you remains: does this ndo_set_mac_address happen from the stack above the driver or not?
Thanks,
sln
Going along with this being the equivalent of the ixgbe patch, I'd
prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
In the design of our drivers, the common file is essentially a device
specific layer, and the OS and platform related stuff should stay in
i40e_main.c. That would also take care of one of the include file nits
that Andy mentioned.
:
I'm not sure about any deeper wisdom, but the HW/FW model that this
device uses is rather different from our previous devices, so our SW
abstractions are a little different from ixgbe and igb.
Ok, in that case I can make i40e_main.c to do something like
static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
:
if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS)
i40e_get_mac_addr(..);
:
}
and i agree that doing this from i40e_main will simplify a lot of the
other stuff around getting the pdev etc.
[Discussion about ndo_set_mac_address..]
In the cases in which I'm aware, the platform manufacturer normally will
burn a different mac-address into the device's eeprom at manufacturing
time if they want something other than what came from Intel. I suppose a
Device Tree oriented platform could have a different address in the DT,
but somehow that needs to get communicated to the device driver so that
it can tell the HW.
My question to you remains: does this ndo_set_mac_address happen from
the stack above the driver or not?
I'm probably even less clued in to the DT arch than you in this case,
so input from the intel experts would be useful here.
But both in this case, and for the ixgbe template on which I tried
to model this, the OF/idprom probing happens from the ->probe when the
driver comes up, and ndo_set_mac_address is not involved.
I dont know if it is easier (or even feasible to do this from ->probe)
to just call the i40e_set_mac to make sure all the state-bits in
the driver are correctly set.
--Sowmini
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: Friday, October 30, 2015 12:25 PM
On (10/30/15 18:57), Nelson, Shannon wrote:
quoted
quoted
quoted
Going along with this being the equivalent of the ixgbe patch, I'd
prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
In the design of our drivers, the common file is essentially a device
specific layer, and the OS and platform related stuff should stay in
i40e_main.c. That would also take care of one of the include file nits
that Andy mentioned.
:
quoted
I'm not sure about any deeper wisdom, but the HW/FW model that this
device uses is rather different from our previous devices, so our SW
abstractions are a little different from ixgbe and igb.
Ok, in that case I can make i40e_main.c to do something like
static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
:
if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS)
i40e_get_mac_addr(..);
:
}
and i agree that doing this from i40e_main will simplify a lot of the
other stuff around getting the pdev etc.
The more common idiom in our driver would be
err = i40e_get_platform_mac_addr(..);
if (err) {
...
}
[Discussion about ndo_set_mac_address..]
quoted
In the cases in which I'm aware, the platform manufacturer normally will
burn a different mac-address into the device's eeprom at manufacturing
time if they want something other than what came from Intel. I suppose a
Device Tree oriented platform could have a different address in the DT,
but somehow that needs to get communicated to the device driver so that
it can tell the HW.
My question to you remains: does this ndo_set_mac_address happen from
the stack above the driver or not?
I'm probably even less clued in to the DT arch than you in this case,
so input from the intel experts would be useful here.
This worries me - if you're not clued into the DT architecture, why are you making these changes? Have you tested this beyond a compile? Do you have a DT model to try this against?
But both in this case, and for the ixgbe template on which I tried
to model this, the OF/idprom probing happens from the ->probe when the
driver comes up, and ndo_set_mac_address is not involved.
I dont know if it is easier (or even feasible to do this from ->probe)
to just call the i40e_set_mac to make sure all the state-bits in
the driver are correctly set.
--Sowmini
In looking at a couple other drivers, I see the difference being that they typically are writing the primary mac filter on probe (and any other reset), whereas the i40e "knows" that the default mac address is already set up as the filter and doesn't bother with a redundant write. If you want to add this Open Filter code, you'll need to arrange for this write to happen. You can't call i40e_set_mac() to do it, but you can see the i40e_aq_mac_address_write() code there that is involved in updating the mac address as an example. You probably will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know how to use i40e_aq_mac_address_write() for your situation.
sln
The more common idiom in our driver would be
err = i40e_get_platform_mac_addr(..);
if (err) {
Ok.
Have you tested this beyond a compile?
Do you have a DT model to try this against?
yes.
In looking at a couple other drivers, I see the difference being that
they typically are writing the primary mac filter on probe (and any
other reset), whereas the i40e "knows" that the default mac address is
already set up as the filter and doesn't bother with a redundant write.
If you want to add this Open Filter code, you'll need to arrange for
this write to happen. You can't call i40e_set_mac() to do it, but you
can see the i40e_aq_mac_address_write() code there that is involved in
updating the mac address as an example. You probably will want to look
at section 4.2.1.5.3 of the XL710 data sheet in order to know how to
use i40e_aq_mac_address_write() for your situation.
ok. I'll look into it (and also why this did not show up in my testing).
fwiw, the ixgbe patch is quite clearly missing in i40e, and hopefully
we wont be opportunistically fixing this one driver at a time in
the future.
--Sowmini
In looking at a couple other drivers, I see the difference being that
they typically are writing the primary mac filter on probe (and any
other reset), whereas the i40e "knows" that the default mac address is
already set up as the filter and doesn't bother with a redundant write.
If you want to add this Open Filter code, you'll need to arrange for
this write to happen. You can't call i40e_set_mac() to do it, but you
can see the i40e_aq_mac_address_write() code there that is involved in
updating the mac address as an example. You probably will want to look
at section 4.2.1.5.3 of the XL710 data sheet in order to know how to
use i40e_aq_mac_address_write() for your situation.
ok. I'll look into it (and also why this did not show up in my testing).
So I figured out why it all "seemed to work" - my test env had another
obscure init process that was marking the link promiscuous. I guess
that was having the side-effect of somehow setting the filters above.
But looks like there's more to getting this right than just calling
i40e_aq_mac_address_write() - I think it also needs a i40e_aq_add_macvlan().
I was able to get this to work by calling a the core part of
i40e_set_mac just before register_netdev. In my patch (RFC patch
in a separate thread - please review) I now have this sequence in
i40e_probe
err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
if (err)
i40e_get_mac_addr(hw, hw->mac.addr);
:
i40e_setup_pf_switch(..);
And the resulting i40e_vsi_setup() from i40e_setup_pf_switch()
will end up doing the right thing by invoking the guts of
i40e_set_mac(), which is basically the sequence:
i40e_aq_mac_address_write()
i40e_aq_add_macvlan()
I dont know if it is necessary/possible/important to set up the
filters sooner in the sequence- the add_macvlan needs an "seid",
and I could not tell when (in the ":" code above) the right seid
can be found.
Please review the RFC patch I'll be sending shortly.
--Sowmini
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: Sunday, November 01, 2015 8:24 AM
[...]
So I figured out why it all "seemed to work" - my test env had another
obscure init process that was marking the link promiscuous. I guess
that was having the side-effect of somehow setting the filters above.
But looks like there's more to getting this right than just calling
i40e_aq_mac_address_write() - I think it also needs a
i40e_aq_add_macvlan().
I was able to get this to work by calling a the core part of
i40e_set_mac just before register_netdev. In my patch (RFC patch
in a separate thread - please review) I now have this sequence in
i40e_probe
err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
if (err)
i40e_get_mac_addr(hw, hw->mac.addr);
:
i40e_setup_pf_switch(..);
And the resulting i40e_vsi_setup() from i40e_setup_pf_switch()
will end up doing the right thing by invoking the guts of
i40e_set_mac(), which is basically the sequence:
i40e_aq_mac_address_write()
i40e_aq_add_macvlan()
I dont know if it is necessary/possible/important to set up the
filters sooner in the sequence- the add_macvlan needs an "seid",
and I could not tell when (in the ":" code above) the right seid
can be found.
Please review the RFC patch I'll be sending shortly.
--Sowmini
Yeah, because of the underlying HW we need to manage, and the fact that we can't ask it for the filters it knows, it becomes a bit convoluted. To manage and replay the filter lists, we use the i40e_{add,del}_filter() routines on the individual VSI filter lists. I'm thinking you're on the right track, but we may not want to bypass the VSI's filter list.
My brain's not in gear this weekend so I'll review the patch later. In the meantime, be sure to test what happens over a reset, such as what happens when the MTU is changed. This will make sure that the replay of mac and vlan filters happens correctly. You'll want to test this with and without vlans.
sln
.. In the meantime, be sure to test what happens over a reset, such as what
happens when the MTU is changed. This will make sure that the replay
of mac and vlan filters happens correctly. You'll want to test this
with and without vlans.
I assume you mean .1q (aka linux macvlan) as opposed to access/trunk vlans?
I will test that tomorrow but I did a quick sanity check on mtu, as well
as turning tso on/off which also restarts the driver (I believe), and
it was "fine", i.e., able to ping offlink hosts.
--Sowmini
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: Sunday, November 01, 2015 4:07 PM
On (11/01/15 21:03), Nelson, Shannon wrote:
quoted
.. In the meantime, be sure to test what happens over a reset, such as
what
quoted
happens when the MTU is changed. This will make sure that the replay
of mac and vlan filters happens correctly. You'll want to test this
with and without vlans.
I assume you mean .1q (aka linux macvlan) as opposed to access/trunk
vlans?
Yes, this is what I had in mind.
I will test that tomorrow but I did a quick sanity check on mtu, as well
as turning tso on/off which also restarts the driver (I believe), and
it was "fine", i.e., able to ping offlink hosts.
--Sowmini
I dont think we're quite there yet, even without vlans.
If I turn on/off tcpdump, there's something about the way that the link
is bounced that leaves the device down while tcpdump is running. Then
after I exit tcpdump, it bounces things a few more times again, packets
flow for a brief interval, and then there's silence.
Seems like there's is a workq that results in
i40e_service_task->i40e_sync_vsi_filters that periodically resets things.
Doing 'ip link set eth0 promisc on' keeps things nice and steady.
How is this all supposed to work if I change the macaddr from /sbin/ip
using i40e_set_mac() and then jiggle the promisc (either just the flag,
or with tcpdump)? (I cant tell because I dont have an x86 machine with
i40e handy)
To frame the question differently, where all should I be invoking
the new i40e_macaddr_init() function from?
--Sowmini
I dont think we're quite there yet, even without vlans.
Ok finally got all the .1q stuff verified (took a bit longer
than it should, because of some lab/administrative hassles in
getting the vlans adjusted at intermediate switches). But
the fix as I'd sent has been tested, and it is good for .1q.
The issues I was seeing with promisc were orthogonal to my fix,
or even to my platform - I understand those are being addressed
separately.
I'm probably too late for the merge window, and I dont know if
this fix qualifies as "critical" for net, so I'll re-target v5
for net-next, to be conservative.
--Sowmini