* Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()
From: Jeff Kirsher @ 2014-11-05 12:34 UTC (permalink / raw)
To: Joe Stringer
Cc: netdev, linux.nics, shahed.shaikh, sathya.perla, amirv,
linux-kernel, Dept-GELinuxNICDev, therbert
In-Reply-To: <1415138202-1197-4-git-send-email-joestringer@nicira.com>
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:
> ndo_gso_check() was recently introduced to allow NICs to report the
> offloading support that they have on a per-skb basis. Add an
> implementation for this driver which checks for something that looks
> like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> Should this driver report support for GSO on packets with tunnel
> headers
> up to 64B like the i40e driver does?
> ---
> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Thanks Joe, I will add your patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()
From: Jeff Kirsher @ 2014-11-05 12:35 UTC (permalink / raw)
To: Joe Stringer
Cc: netdev, sathya.perla, linux.nics, amirv, shahed.shaikh,
Dept-GELinuxNICDev, therbert, linux-kernel
In-Reply-To: <1415138202-1197-3-git-send-email-joestringer@nicira.com>
[-- Attachment #1: Type: text/plain, Size: 673 bytes --]
On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:
> ndo_gso_check() was recently introduced to allow NICs to report the
> offloading support that they have on a per-skb basis. Add an
> implementation for this driver which checks for tunnel headers over
> UDP
> of up to 64 octets in length.
>
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
Thanks again Joe, I will add your patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net 2/5] i40e: Implement ndo_gso_check()
From: Jeff Kirsher @ 2014-11-05 12:37 UTC (permalink / raw)
To: Jesse Gross, Shannon Nelson, Jesse Brandeburg
Cc: Joe Stringer, netdev, Sathya Perla, linux.nics, amirv,
shahed.shaikh, Dept-GELinuxNICDev, Tom Herbert,
Linux Kernel Mailing List
In-Reply-To: <CAEP_g=8rbJF5z92LV_c14KooU=i3x=9WpikocctcEUOdw5-oLA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]
On Tue, 2014-11-04 at 15:45 -0800, Jesse Gross wrote:
> On Tue, Nov 4, 2014 at 1:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index c3a7f4a..21829b5 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +static bool i40e_gso_check(struct sk_buff *skb, struct net_device *dev)
> > +{
> > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > + skb->inner_protocol != htons(ETH_P_TEB) ||
> > + skb_inner_mac_header(skb) - skb_transport_header(skb) > 64))
> > + return false;
>
> I think it may be possible to even support a few more things here.
> According to the datasheet here:
> http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xl710-10-40-controller-datasheet.pdf
>
> This can actually support 64 bytes beyond the tunnel header, which
> would make for a total of 80 bytes. It looks like it can also support
> IPv4 or IPv6 beyond just Ethernet as the encapsulated protocol.
>
> Intel guys, can you confirm that this is correct?
I believe you are correct Jesse, but I will let Shannon Nelson or Jesse
Brandeburg respond since they are the i40e maintainers.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net 0/5] Implement ndo_gso_check() for vxlan nics
From: Or Gerlitz @ 2014-11-05 12:38 UTC (permalink / raw)
To: Joe Stringer
Cc: Linux Netdev List, sathya.perla, Jeff Kirsher, linux.nics,
Amir Vadai, shahed.shaikh, Dept-GELinuxNICDev, Tom Herbert,
Linux Kernel
In-Reply-To: <1415138202-1197-1-git-send-email-joestringer@nicira.com>
On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not other
> UDP-based encapsulation protocols where the format and size of the header may
> differ. This patch series implements ndo_gso_check() for these NICs,
> restricting the GSO handling to something that looks and smells like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert (with minor fixups):
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
Hi Joe,
1st, thanks for picking this task...2nd, for drivers that currently
support only pure VXLAN, I don't see the point
to replicate the helper suggested by Tom (good catch on the size check
to be 16 and not 12) four times and who know how more in the future.
Let's just have one generic helper and make the mlx4/be/fm10k/benet
drivers to have it as their ndo, OK?
Or.
>
> If there are particular differences for your driver on actual support, I'd like
> to hear about it. I adjusted the i40e driver to report support with tunnel
> headers of up to 64 octets, perhaps there are other specifics that I've missed.
>
> Joe Stringer (5):
> be2net: Implement ndo_gso_check()
> i40e: Implement ndo_gso_check()
> fm10k: Implement ndo_gso_check()
> net/mlx4_en: Implement ndo_gso_check()
> qlcnic: Implement ndo_gso_check()
>
> drivers/net/ethernet/emulex/benet/be_main.c | 12 ++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++
> drivers/net/ethernet/intel/i40e/i40e_main.c | 14 +++++++++++++-
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 12 ++++++++++++
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 12 ++++++++++++
> 5 files changed, 61 insertions(+), 1 deletion(-)
>
> --
> 1.7.10.4
> --
> 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
^ permalink raw reply
* Re: "asix: Don't reset PHY on if_up for ASIX 88772" breaks net onarndale platform
From: Riku Voipio @ 2014-11-05 12:39 UTC (permalink / raw)
To: Stam, Michel [FINT]
Cc: Charles Keepax, davem, linux-usb, netdev, linux-kernel,
linux-samsung-soc
In-Reply-To: <C89EFD3CD56F64468D3D206D683A8D22039FFC5F@ldam-msx2.fugro-nl.local>
Hi Michel,
On Wed, Nov 05, 2014 at 01:04:37PM +0100, Stam, Michel [FINT] wrote:
> After looking around I found the reset value for the 8772 chip, which
> seems to be 0x1E1 (ANAR register).
>
> This equates to (according to include/uapi/linux/mii.h)
> ADVERTISE_ALL | ADVERTISE_CSMA.
>
> The register only seems to become 0 if the software reset fails.
>
> Unfortunately, this is exactly what I get when the patch is applied;
> asix 1-2:1.0 eth1: Failed to send software reset: ffffffb5
> asix 1-2:1.0 eth1: link reset failed (-75) usbnet usb-0000:00:1d.0-2,
> ASIX AX88772 USB 2.0 Ethernet
> asix 1-2:1.0 eth1: Failed to send software reset: ffffffb5
> asix 1-2:1.0 eth1: link reset failed (-75) usbnet usb-0000:00:1d.0-2,
> ASIX AX88772 USB 2.0 Ethernet
>
> A little while after this its 'Failed to enable software MII access'.
> The ethernet device fails to see any link or accept any ethtool -s
> command.
> My device has vid:devid 0b95:772a (ASIX Elec. Corp.).
> Can you tell me what device is on the Andale platform, Charles? Same
> vendor/device id?
Bus 003 Device 004: ID 0b95:772a ASIX Electronics Corp. AX88772A Fast Ethernet
> Another thing that bothers me is that dev->mii.advertising seems to
> contain the same value, so maybe that can be used instead of a call to
> asix_mdio_read(). Can anyone comment on its purpose? Should it be a
> shadow copy of the real register or something?
> Riku, can you test Charles' patch as well?
With that patch + revert to ax88772_reset network works. I'm unable to get
ethtool to work with that patch or with the original 3.17 state of asix.
Once i disable autoneg network doesn't just work.
> > >I think it would better to revert the change now and with less hurry
> > introduce a ethtool fix that doesn't break arndale.
> > I don't fully agree here;
> > I would like to point out that this commit is a revert itself. Fixing
> > the armdale will then cause breakage in other implementations, such as
> > ours. Blankly reverting breaks other peoples' implementations.
My concern is, that none of us with this problem is a linux network
drivers expert. And no such expert joined the thread to help us. Thus if
we hurry to have proper fix for 3.18, our fix might easily be really
wrong.
Hence, it would seem safer to revert to 3.17 state before 3.18, so we
can propose a proper fix for 3.19. At least from our myopic view, having
no functioning net on arndale is worse than having non-functioning
ethtool (which doesn't seem to have bothered people for years).
Riku
> > The PHY reset is the thing that breaks ethtool support, so any fix
> > that appeases all would have to take existing PHY state into account.
> >
> > I'm not an expert on the ASIX driver, nor the MII, but I think this is
>
> > the cause;
> > drivers/net/usb/asix_devices.c:
> > 361 asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR,
> > BMCR_RESET);
> > 362 asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE,
> > 363 ADVERTISE_ALL | ADVERTISE_CSMA);
> > 364 mii_nway_restart(&dev->mii);
> >
> > I would think that the ADVERTISE_ALL is the cause here, as it will
> > reset the MII back to default, thus overriding ethtool settings.
> > Would an:
> > Int reg;
> > reg = asix_mdio_read(dev->net,dev->mii.phy_id,MII_ADVERTISE);
> >
> > prior to the offending lines, and then;
> >
> > 362 asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE,
> > 363 reg);
> >
> > solve the problem for you guys?
>
> If I revert the patch in question and add this in:
>
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -299,6 +299,7 @@ static int ax88772_reset(struct usbnet *dev) {
> struct asix_data *data = (struct asix_data *)&dev->data;
> int ret, embd_phy;
> + int reg;
> u16 rx_ctl;
>
> ret = asix_write_gpio(dev,
> @@ -359,8 +360,10 @@ static int ax88772_reset(struct usbnet *dev)
> msleep(150);
>
> asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR,
> BMCR_RESET);
> - asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE,
> - ADVERTISE_ALL | ADVERTISE_CSMA);
> + reg = asix_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE);
> + if (!reg)
> + reg = ADVERTISE_ALL | ADVERTISE_CSMA;
> + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, reg);
> mii_nway_restart(&dev->mii);
>
> ret = asix_write_medium_mode(dev, AX88772_MEDIUM_DEFAULT);
>
> Then things work on Arndale for me. Does that work for you?
> Whether that is a sensible fix I don't know however.
>
> >
> > Mind, maybe the read function should take into account the reset value
>
> > of the MII, and set it to ADVERTISE_ALL | ADVERTISE_CSMA. I don't have
>
> > any documention here at the moment.
>
> Yeah I also have no documentation.
>
> Thanks,
> Charles
>
> >
> > Is anyone able to confirm my suspicions?
> >
> > Kind regards,
> >
> > Michel Stam
> > -----Original Message-----
> > From: Riku Voipio [mailto:riku.voipio@iki.fi]
> > Sent: Tuesday, November 04, 2014 10:44 AM
> > To: Stam, Michel [FINT]
> > Cc: Riku Voipio; davem@davemloft.net; linux-usb@vger.kernel.org;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-samsung-soc@vger.kernel.org; ckeepax@opensource.wolfsonmicro.com
> > Subject: Re: "asix: Don't reset PHY on if_up for ASIX 88772" breaks
> > net on arndale platform
> >
> > On Tue, Nov 04, 2014 at 09:19:26AM +0100, Stam, Michel [FINT] wrote:
> > > Interesting, as the commit itself is a revert from a kernel back to
> > > 2.6 somewhere. The problem I had is related to the PHY being reset
> > > on interface-up, can you confirm that you require this?
> >
> > I can't confirm what exactly is needed on arndale. I'm neither expert
> > in USB or ethernet. However, I can confirm that without the PHY reset,
>
> > networking doesn't work on arndale.
> >
> > I now see someone else has the same problem, adding Charles to CC.
> >
> > http://www.spinics.net/lists/linux-usb/msg116656.html
> >
> > > Reverting this
> > > breaks ethtool support in turn.
> >
> > Fixing a bug (ethtool support) must not cause breakage elsewhere (in
> > this case on arndale). This is now a regression of functionality from
> > 3.17.
> >
> > I think it would better to revert the change now and with less hurry
> > introduce a ethtool fix that doesn't break arndale.
> >
> > > Kind regards,
> > >
> > > Michel Stam
> > >
> > > -----Original Message-----
> > > From: Riku Voipio [mailto:riku.voipio@iki.fi]
> > > Sent: Tuesday, November 04, 2014 8:23 AM
> > > To: davem@davemloft.net; Stam, Michel [FINT]
> > > Cc: linux-usb@vger.kernel.org; netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> > > Subject: "asix: Don't reset PHY on if_up for ASIX 88772" breaks net
> > > on
> >
> > > arndale platform
> > >
> > > Hi,
> > >
> > > With 3.18-rc3, asix on arndale (samsung exynos 5250 based board),
> > > fails to work. Interface is initialized but network traffic seem not
>
> > > to pass through. With kernel IP config the result looks like:
> > >
> > > [ 3.323275] usb 3-3.2.4: new high-speed USB device number 4 using
> > > exynos-ehci
> > > [ 3.419151] usb 3-3.2.4: New USB device found, idVendor=0b95,
> > > idProduct=772a
> > > [ 3.424735] usb 3-3.2.4: New USB device strings: Mfr=1,
> Product=2,
> > > SerialNumber=3
> > > [ 3.432196] usb 3-3.2.4: Product: AX88772
> > > [ 3.436279] usb 3-3.2.4: Manufacturer: ASIX Elec. Corp.
> > > [ 3.441486] usb 3-3.2.4: SerialNumber: 000001
> > > [ 3.447530] asix 3-3.2.4:1.0 (unnamed net_device)
> (uninitialized):
> > > invalid hw address, using random
> > > [ 3.764352] asix 3-3.2.4:1.0 eth0: register 'asix' at
> > > usb-12110000.usb-3.2.4, ASIX AX88772 USB 2.0 Ethernet,
> > de:a2:66:bf:ca:4f
> > > [ 4.488773] asix 3-3.2.4:1.0 eth0: link down
> > > [ 5.690025] asix 3-3.2.4:1.0 eth0: link up, 100Mbps, full-duplex,
> > lpa
> > > 0xC5E1
> > > [ 5.712947] Sending DHCP requests ...... timed out!
> > > [ 83.165303] IP-Config: Retrying forever (NFS root)...
> > > [ 83.170397] asix 3-3.2.4:1.0 eth0: link up, 100Mbps, full-duplex,
> > lpa
> > > 0xC5E1
> > > [ 83.192944] Sending DHCP requests .....
> > >
> > > Similar results also with dhclient. Git bisect identified the
> > > breaking
> >
> > > commit as:
> > >
> > > commit 3cc81d85ee01e5a0b7ea2f4190e2ed1165f53c31
> > > Author: Michel Stam <m.stam@fugro.nl>
> > > Date: Thu Oct 2 10:22:02 2014 +0200
> > >
> > > asix: Don't reset PHY on if_up for ASIX 88772
> > >
> > > Taking 3.18-rc3 and that commit reverted, network works again:
> > >
> > > [ 3.303500] usb 3-3.2.4: new high-speed USB device number 4 using
> > > exynos-ehci
> > > [ 3.399375] usb 3-3.2.4: New USB device found, idVendor=0b95,
> > > idProduct=772a
> > > [ 3.404963] usb 3-3.2.4: New USB device strings: Mfr=1,
> Product=2,
> > > SerialNumber=3
> > > [ 3.412424] usb 3-3.2.4: Product: AX88772
> > > [ 3.416508] usb 3-3.2.4: Manufacturer: ASIX Elec. Corp.
> > > [ 3.421715] usb 3-3.2.4: SerialNumber: 000001
> > > [ 3.427755] asix 3-3.2.4:1.0 (unnamed net_device)
> (uninitialized):
> > > invalid hw address, using random
> > > [ 3.744837] asix 3-3.2.4:1.0 eth0: register 'asix' at
> > > usb-12110000.usb-3.2.4, ASIX AX88772 USB 2.0 Ethernet,
> > 12:59:f1:a8:43:90
> > > [ 7.098998] asix 3-3.2.4:1.0 eth0: link up, 100Mbps, full-duplex,
> > lpa
> > > 0xC5E1
> > > [ 7.118258] Sending DHCP requests ., OK
> > > [ 9.753259] IP-Config: Got DHCP answer from 192.168.1.1, my
> address
> > > is 192.168.1.111
> > >
> > > There might something wrong on the samsung platform code (I
> > > understand
> >
> > > the USB on arndale is "funny"), but this is still an regression from
>
> > > 3.17.
> > >
> > > Riku
^ permalink raw reply
* Re: [PATCH net 4/5] net/mlx4_en: Implement ndo_gso_check()
From: Or Gerlitz @ 2014-11-05 12:41 UTC (permalink / raw)
To: Joe Stringer
Cc: Linux Netdev List, sathya.perla, Jeff Kirsher, linux.nics,
Amir Vadai, shahed.shaikh, Dept-GELinuxNICDev, Tom Herbert,
Linux Kernel
In-Reply-To: <1415138202-1197-5-git-send-email-joestringer@nicira.com>
On Tue, Nov 4, 2014 at 11:56 PM, Joe Stringer <joestringer@nicira.com> wrote:
> ndo_gso_check() was recently introduced to allow NICs to report the
> offloading support that they have on a per-skb basis. Add an
> implementation for this driver which checks for something that looks
> like VXLAN.
>
> Implementation shamelessly stolen from Tom Herbert:
> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index f3032fe..aca9908 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2344,6 +2344,17 @@ static void mlx4_en_del_vxlan_port(struct net_device *dev,
> }
> #endif
>
> +static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
> +{
> + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> + (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> + skb->inner_protocol != htons(ETH_P_TEB) ||
> + skb_inner_mac_header(skb) - skb_transport_header(skb) != 16))
> + return false;
Let's have this 16 constant to be more clear... e.g make it the sum of
sizeof struct udphdr and struct vxlanhdr - you would need to move the
latter from vxlan.c into a public header. All for the general patch I
suggested
Or.
> +
> + return true;
> +}
> +
> static const struct net_device_ops mlx4_netdev_ops = {
> .ndo_open = mlx4_en_open,
> .ndo_stop = mlx4_en_close,
> @@ -2374,6 +2385,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
> .ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
> .ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
> #endif
> + .ndo_gso_check = mlx4_en_gso_check,
> };
>
> static const struct net_device_ops mlx4_netdev_ops_master = {
> --
> 1.7.10.4
>
> --
> 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
^ permalink raw reply
* Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()
From: Or Gerlitz @ 2014-11-05 12:44 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Joe Stringer, Linux Netdev List, linux.nics, shahed.shaikh,
sathya.perla, Amir Vadai, Linux Kernel, Dept-GELinuxNICDev,
Tom Herbert
In-Reply-To: <1415190872.2420.55.camel@jtkirshe-mobl>
On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:
>> ndo_gso_check() was recently introduced to allow NICs to report the
>> offloading support that they have on a per-skb basis. Add an
>> implementation for this driver which checks for something that looks
>> like VXLAN.
>>
>> Implementation shamelessly stolen from Tom Herbert:
>> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
>>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>> Should this driver report support for GSO on packets with tunnel
>> headers
>> up to 64B like the i40e driver does?
>> ---
>> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>
> Thanks Joe, I will add your patch to my queue.
Hi Jeff, please see my comment on patch 0/5, we're essentially
replicating the same helper four different times (fm10k, mlx4, benet,
qlgc) - I don't see the point in doing so. I asked Joe to come up with
one generic helper and then to pick it up by the four drivers, makes
sense?
^ permalink raw reply
* Re: [linux-nics] [PATCH net 3/5] fm10k: Implement ndo_gso_check()
From: Jeff Kirsher @ 2014-11-05 12:47 UTC (permalink / raw)
To: Or Gerlitz
Cc: Joe Stringer, Linux Netdev List, linux.nics, shahed.shaikh,
sathya.perla, Amir Vadai, Linux Kernel, Dept-GELinuxNICDev,
Tom Herbert
In-Reply-To: <CAJ3xEMjaQ54yEUQGL-8VHoPm=4G5bhPaGBa55jypzfKviR4KyQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]
On Wed, 2014-11-05 at 14:44 +0200, Or Gerlitz wrote:
> On Wed, Nov 5, 2014 at 2:34 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > On Tue, 2014-11-04 at 13:56 -0800, Joe Stringer wrote:
> >> ndo_gso_check() was recently introduced to allow NICs to report the
> >> offloading support that they have on a per-skb basis. Add an
> >> implementation for this driver which checks for something that looks
> >> like VXLAN.
> >>
> >> Implementation shamelessly stolen from Tom Herbert:
> >> http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >>
> >> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> >> ---
> >> Should this driver report support for GSO on packets with tunnel
> >> headers
> >> up to 64B like the i40e driver does?
> >> ---
> >> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >
> > Thanks Joe, I will add your patch to my queue.
>
> Hi Jeff, please see my comment on patch 0/5, we're essentially
> replicating the same helper four different times (fm10k, mlx4, benet,
> qlgc) - I don't see the point in doing so. I asked Joe to come up with
> one generic helper and then to pick it up by the four drivers, makes
> sense?
Yeah, I just saw your reply Or. Ok, I will await an update to Joe's
series, thanks!
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Dong Aisheng @ 2014-11-05 12:47 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-can, mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <545A21AD.5040608@hartkopp.net>
On Wed, Nov 05, 2014 at 02:10:05PM +0100, Oliver Hartkopp wrote:
> On 05.11.2014 12:26, Dong Aisheng wrote:
> >On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> >>On 05.11.2014 08:58, Dong Aisheng wrote:
>
>
> >>Unfortunately No. Here it becomes complicated due to the fact that
> >>the revision 3.0.x does not support per-frame switching for FD/BRS
> >>...
> >
> >I'm not sure i got your point.
> > From m_can spec, it allows switch CAN mode by setting CMR bit.
> >
> >Bits 11:10 CMR[1:0]: CAN Mode Request
> >A change of the CAN operation mode is requested by writing to this bit field. After change to the
> >requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set
> >accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
> >retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a
> >change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
> >according to ISO11898-1.
> >00 unchanged
> >01 Request CAN FD operation
> >10 Request CAN FD operation with bit rate switching
> >11 Request CAN operation according ISO11898-1
> >
> >So what's the difference between this function and the per-frame switching
> >you mentioned?
> >
> >>
> >>When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> >>tells us, that the controller is _capable_ to send either CAN or CAN
> >>FD frames.
> >>
> >>It does not configure the controller into one of these specific settings!
> >>
> >>Additionally: AFAIK when writing to the CCCR you have to make sure
> >>that there's currently no ongoing transfer.
> >>
> >
> >I don't know about it before.
> >By searching m_can user manual v302 again, i still did not find this
> >limitation. Can you point me if you know where it is?
> >
> >BTW, since we only use one Tx Buffer for transmission currently, so we
> >should not meet that case that CAN mode is switched during transfer.
> >So the issue you concern may not happen.
>
> Yes. You are right. Having a FIFO with a size of 1 will help here :-)
>
> >
> >Additionally it looks to me like m_can does allow set CMR bit at runtime
> >since the mode change will take affect when controller becomes idle.
> >See below:
> >
> >"A mode change requested by writing to CCCR.CMR will be executed next
> >time the CAN protocol controller FSM reaches idle phase between CAN frames.
> >Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS
> >and CCCR.FDO are set accordingly. In case the requested
> >CAN operation mode is not enabled, the value written to CCCR.CMR is
> >retained until it is overwritten by the next mode change request.
> >Default is CAN operation according to ISO11898-1."
>
> Ah. I assumed we have to take care to set these bits in the idle state.
>
> So when thinking about the one and only tx buffer your current
> approach should work indeed. Sorry for my confusion.
>
> >
> >>I would suggest the following approach to make the revision 3.0.x
> >>act like a correct CAN FD capable controller:
> >>
> >>- create a helper function to switch FD and BRS while taking care of
> >>ongoing transmissions
> >>
> >>- create a variable that knows the current tx_mode for FD and BRS
> >>
> >>When we need to send a CAN frame which doesn't fit the settings in
> >>the tx_mode we need to switch the CCCR and update the tx_mode
> >>variable.
> >>
> >>This at least reduces the needed CCCR operations.
> >>
> >
> >Yes, i can do like that.
> >But what i see by doing that is only reduces the needed CCCR operations?
> >Any other benefits i missed?
>
> No. I just thought about a function that also takes care of the idle
> phase to switch the bits. But now you made it clear that this is not
> needed - so you can stay on your current implementation: Setting the
> CCCR each time before sending the frame.
>
Okay
> With the 3.1.x or 3.2.x this code will become obsolete then as the
> EDL and BRS seeting will get extra bits in the Tx Buffer.
> But that's a future point.
>
Got it.
Will update it in the future when the new IP doc is available.
> >And the test for current code showed it seemed work well on the Mode
> >switch among std frame/fd frame/brs frame.
>
> Fine.
>
> Btw. I would suggest to integrate patch [4/4] into [3/4].
>
Yes, will do it.
Thanks
Regards
Dong Aisheng
> Best regards,
> Oliver
>
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Dong Aisheng @ 2014-11-05 12:47 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-can, mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <545A22EF.3070509@hartkopp.net>
On Wed, Nov 05, 2014 at 02:15:27PM +0100, Oliver Hartkopp wrote:
> Typo
>
> On 05.11.2014 14:10, Oliver Hartkopp wrote:
>
> >Btw. I would suggest to integrate patch [4/4] into [3/4].
>
> into [1/4] of course
Understand! :-)
Regards
Dong Aisheng
^ permalink raw reply
* [net PATCH 1/1] drivers: net: cpsw: remove cpsw_ale_stop from cpsw_ale_destroy
From: Mugunthan V N @ 2014-11-05 13:03 UTC (permalink / raw)
To: netdev; +Cc: davem, Mugunthan V N
when cpsw is build as modulea and simple insert and removal of module
creates a deadlock, due to delete timer. the timer is created and destroyed
in cpsw_ale_start and cpsw_ale_stop which are from device open and close.
root@am437x-evm:~# modprobe -r ti_cpsw
[ 158.505333] INFO: trying to register non-static key.
[ 158.510623] the code is fine but needs lockdep annotation.
[ 158.516448] turning off the locking correctness validator.
[ 158.522282] CPU: 0 PID: 1339 Comm: modprobe Not tainted 3.14.23-00445-gd41c88f #44
[ 158.530359] [<c0015380>] (unwind_backtrace) from [<c0012088>] (show_stack+0x10/0x14)
[ 158.538603] [<c0012088>] (show_stack) from [<c054ad70>] (dump_stack+0x78/0x94)
[ 158.546295] [<c054ad70>] (dump_stack) from [<c0088008>] (__lock_acquire+0x176c/0x1b74)
[ 158.554711] [<c0088008>] (__lock_acquire) from [<c0088944>] (lock_acquire+0x9c/0x104)
[ 158.563043] [<c0088944>] (lock_acquire) from [<c004e520>] (del_timer_sync+0x44/0xd8)
[ 158.571289] [<c004e520>] (del_timer_sync) from [<bf2eac1c>] (cpsw_ale_destroy+0x10/0x3c [ti_cpsw])
[ 158.580821] [<bf2eac1c>] (cpsw_ale_destroy [ti_cpsw]) from [<bf2eb268>] (cpsw_remove+0x30/0xa0 [ti_cpsw])
[ 158.591000] [<bf2eb268>] (cpsw_remove [ti_cpsw]) from [<c035ef44>] (platform_drv_remove+0x18/0x1c)
[ 158.600527] [<c035ef44>] (platform_drv_remove) from [<c035d8bc>] (__device_release_driver+0x70/0xc8)
[ 158.610236] [<c035d8bc>] (__device_release_driver) from [<c035e0d4>] (driver_detach+0xb4/0xb8)
[ 158.619386] [<c035e0d4>] (driver_detach) from [<c035d6e4>] (bus_remove_driver+0x4c/0x90)
[ 158.627988] [<c035d6e4>] (bus_remove_driver) from [<c00af2a8>] (SyS_delete_module+0x10c/0x198)
[ 158.637144] [<c00af2a8>] (SyS_delete_module) from [<c000e580>] (ret_fast_syscall+0x0/0x48)
[ 179.524727] INFO: rcu_sched detected stalls on CPUs/tasks: {} (detected by 0, t=2102 jiffies, g=1487, c=1486, q=6)
[ 179.535741] INFO: Stall ended before state dump start
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/net/ethernet/ti/cpsw_ale.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index 3ae8387..097ebe7 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -785,7 +785,6 @@ int cpsw_ale_destroy(struct cpsw_ale *ale)
{
if (!ale)
return -EINVAL;
- cpsw_ale_stop(ale);
cpsw_ale_control_set(ale, 0, ALE_ENABLE, 0);
kfree(ale);
return 0;
--
2.1.2.484.g13da0fc
^ permalink raw reply related
* Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function
From: Eyal perry @ 2014-11-05 13:05 UTC (permalink / raw)
To: Or Gerlitz, Eyal Perry
Cc: Amir Vadai, David S. Miller, Ben Hutchings, netdev,
Yevgeny Petrilin
In-Reply-To: <545A17C2.9070609@mellanox.com>
On 11/5/2014 2:27 PM, Or Gerlitz wrote:
> On 11/5/2014 1:59 PM, Amir Vadai wrote:
>> From: Eyal Perry <eyalpe@mellanox.com>
>>
>> The ConnectX HW is capable of using one of the following hash functions:
>> Toeplitz and an XOR hash function.
>> This patch implements ethtool_ops {get,set}_rxfh_func callbacks for the
>> mlx4_en driver in order to query and configure the RSS hash function to
>> be used by the device.
>>
>> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>> drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 40
>> +++++++++++++++++++++++++
>> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 9 ++++++
>> drivers/net/ethernet/mellanox/mlx4/en_rx.c | 11 ++++---
>> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 3 +-
>> 4 files changed, 58 insertions(+), 5 deletions(-)
>>
[...]
>> +
>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>> +{
>> + struct mlx4_en_priv *priv = netdev_priv(dev);
>> + struct mlx4_en_dev *mdev = priv->mdev;
>> + u32 prev_rss_hash_fn = priv->rss_hash_fn;
>> + int err = 0;
>> +
>> + if (!(hfunc & priv->rss_hash_fn_caps))
>> + return -EINVAL;
>> +
>> + priv->rss_hash_fn = hfunc;
>
> Seems that you are blindly setting here priv->rss_hash_fn to the
> function (xor or top) requested
> by the user w.o prior checking if the firmware actually supports that
> (e.g test it against priv->rss_hash_fn_caps, right?
>
That exactly what I do 3 lines above, priv->rss_hash_fn_caps is derived
from firmware capabilities.
[...]
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>> mlx4_en_priv *priv)
>> }
>> rss_context->flags = rss_mask;
>> - rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>> - for (i = 0; i < 10; i++)
>> - rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>> -
>> + if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>> + rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>> + } else {
>> + rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>> + for (i = 0; i < 10; i++)
>> + rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>> + }
>
> So this patch effectively changes the default from top to xor, right? is
> that intentional? if yes, spare few words on the change log.
>
No, in general the default shouldn't be affected by the patch (unless
there's a bug). The default value is set in mlx4_en_init_netdev() and it
will remain Toeplitz unless firmware is supports only XOR hash function.
[...]
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Oliver Hartkopp @ 2014-11-05 13:10 UTC (permalink / raw)
To: Dong Aisheng; +Cc: linux-can, mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <20141105112639.GB4007@shlinux1.ap.freescale.net>
On 05.11.2014 12:26, Dong Aisheng wrote:
> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
>> On 05.11.2014 08:58, Dong Aisheng wrote:
>> Unfortunately No. Here it becomes complicated due to the fact that
>> the revision 3.0.x does not support per-frame switching for FD/BRS
>> ...
>
> I'm not sure i got your point.
> From m_can spec, it allows switch CAN mode by setting CMR bit.
>
> Bits 11:10 CMR[1:0]: CAN Mode Request
> A change of the CAN operation mode is requested by writing to this bit field. After change to the
> requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set
> accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is
> retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a
> change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation
> according to ISO11898-1.
> 00 unchanged
> 01 Request CAN FD operation
> 10 Request CAN FD operation with bit rate switching
> 11 Request CAN operation according ISO11898-1
>
> So what's the difference between this function and the per-frame switching
> you mentioned?
>
>>
>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
>> tells us, that the controller is _capable_ to send either CAN or CAN
>> FD frames.
>>
>> It does not configure the controller into one of these specific settings!
>>
>> Additionally: AFAIK when writing to the CCCR you have to make sure
>> that there's currently no ongoing transfer.
>>
>
> I don't know about it before.
> By searching m_can user manual v302 again, i still did not find this
> limitation. Can you point me if you know where it is?
>
> BTW, since we only use one Tx Buffer for transmission currently, so we
> should not meet that case that CAN mode is switched during transfer.
> So the issue you concern may not happen.
Yes. You are right. Having a FIFO with a size of 1 will help here :-)
>
> Additionally it looks to me like m_can does allow set CMR bit at runtime
> since the mode change will take affect when controller becomes idle.
> See below:
>
> "A mode change requested by writing to CCCR.CMR will be executed next
> time the CAN protocol controller FSM reaches idle phase between CAN frames.
> Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS
> and CCCR.FDO are set accordingly. In case the requested
> CAN operation mode is not enabled, the value written to CCCR.CMR is
> retained until it is overwritten by the next mode change request.
> Default is CAN operation according to ISO11898-1."
Ah. I assumed we have to take care to set these bits in the idle state.
So when thinking about the one and only tx buffer your current approach should
work indeed. Sorry for my confusion.
>
>> I would suggest the following approach to make the revision 3.0.x
>> act like a correct CAN FD capable controller:
>>
>> - create a helper function to switch FD and BRS while taking care of
>> ongoing transmissions
>>
>> - create a variable that knows the current tx_mode for FD and BRS
>>
>> When we need to send a CAN frame which doesn't fit the settings in
>> the tx_mode we need to switch the CCCR and update the tx_mode
>> variable.
>>
>> This at least reduces the needed CCCR operations.
>>
>
> Yes, i can do like that.
> But what i see by doing that is only reduces the needed CCCR operations?
> Any other benefits i missed?
No. I just thought about a function that also takes care of the idle phase to
switch the bits. But now you made it clear that this is not needed - so you
can stay on your current implementation: Setting the CCCR each time before
sending the frame.
With the 3.1.x or 3.2.x this code will become obsolete then as the EDL and BRS
seeting will get extra bits in the Tx Buffer.
But that's a future point.
> And the test for current code showed it seemed work well on the Mode
> switch among std frame/fd frame/brs frame.
Fine.
Btw. I would suggest to integrate patch [4/4] into [3/4].
Best regards,
Oliver
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Oliver Hartkopp @ 2014-11-05 13:15 UTC (permalink / raw)
To: Dong Aisheng; +Cc: linux-can, mkl, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <545A21AD.5040608@hartkopp.net>
Typo
On 05.11.2014 14:10, Oliver Hartkopp wrote:
> Btw. I would suggest to integrate patch [4/4] into [3/4].
into [1/4] of course
^ permalink raw reply
* [PATCH V3 1/3] can: add can_is_canfd_skb() API
From: Dong Aisheng @ 2014-11-05 13:16 UTC (permalink / raw)
To: linux-can
Cc: mkl, wg, varkabhadram, netdev, socketcan, b29396,
linux-arm-kernel
The CAN device drivers can use it to check if the frame to send is on
CAN FD mode or normal CAN mode.
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangesLog:
* v1->v2: change to skb->len == CANFD_MTU;
---
include/linux/can/dev.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6992afc..4c3919c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -99,6 +99,11 @@ inval_skb:
return 1;
}
+static inline int can_is_canfd_skb(struct sk_buff *skb)
+{
+ return skb->len == CANFD_MTU;
+}
+
/* get data length from can_dlc with sanitized can_dlc */
u8 can_dlc2len(u8 can_dlc);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Marc Kleine-Budde @ 2014-11-05 13:19 UTC (permalink / raw)
To: Oliver Hartkopp, Dong Aisheng
Cc: linux-can, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <545A21AD.5040608@hartkopp.net>
[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]
On 11/05/2014 02:10 PM, Oliver Hartkopp wrote:
> On 05.11.2014 12:26, Dong Aisheng wrote:
>> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
>>> On 05.11.2014 08:58, Dong Aisheng wrote:
>
>
>>> Unfortunately No. Here it becomes complicated due to the fact that
>>> the revision 3.0.x does not support per-frame switching for FD/BRS
>>> ...
>>
>> I'm not sure i got your point.
>> From m_can spec, it allows switch CAN mode by setting CMR bit.
>>
>> Bits 11:10 CMR[1:0]: CAN Mode Request
>> A change of the CAN operation mode is requested by writing to this bit
>> field. After change to the
>> requested operation mode the bit field is reset to “00” and the status
>> flags FDBS and FDO are set
>> accordingly. In case the requested CAN operation mode is not enabled,
>> the value written to CMR is
>> retained until it is overwritten by the next mode change request. In
>> case CME = “01”/”10”/”11” a
>> change to CAN operation according to ISO 11898-1 is always possible.
>> Default is CAN operation
>> according to ISO11898-1.
>> 00 unchanged
>> 01 Request CAN FD operation
>> 10 Request CAN FD operation with bit rate switching
>> 11 Request CAN operation according ISO11898-1
>>
>> So what's the difference between this function and the per-frame
>> switching
>> you mentioned?
>>
>>>
>>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
>>> tells us, that the controller is _capable_ to send either CAN or CAN
>>> FD frames.
>>>
>>> It does not configure the controller into one of these specific
>>> settings!
>>>
>>> Additionally: AFAIK when writing to the CCCR you have to make sure
>>> that there's currently no ongoing transfer.
>>>
>>
>> I don't know about it before.
>> By searching m_can user manual v302 again, i still did not find this
>> limitation. Can you point me if you know where it is?
>>
>> BTW, since we only use one Tx Buffer for transmission currently, so we
>> should not meet that case that CAN mode is switched during transfer.
>> So the issue you concern may not happen.
>
> Yes. You are right. Having a FIFO with a size of 1 will help here :-)
Errrr....sorry...no.
Taking an easy route here but making it x times harder to extend the
driver to make use of the FIFO is not an option.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 6/8] net: can: c_can: Disable pins when CAN interface is down
From: Marc Kleine-Budde @ 2014-11-05 13:24 UTC (permalink / raw)
To: Roger Quadros, wg
Cc: wsa, tony, tglx, mugunthanvnm, george.cherian, balbi, nsekhar, nm,
sergei.shtylyov, linux-omap, linux-can, netdev
In-Reply-To: <1415096461-25576-7-git-send-email-rogerq@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]
On 11/04/2014 11:20 AM, Roger Quadros wrote:
> DRA7 CAN IP suffers from a problem which causes it to be prevented
> from fully turning OFF (i.e. stuck in transition) if the module was
> disabled while there was traffic on the CAN_RX line.
>
> To work around this issue we select the SLEEP pin state by default
> on probe and use the DEFAULT pin state on CAN up and back to the
> SLEEP pin state on CAN down.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> drivers/net/can/c_can/c_can.c | 20 ++++++++++++++++++++
> drivers/net/can/c_can/c_can.h | 1 +
> drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 8e78bb4..4dfc3ce 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -35,6 +35,7 @@
> #include <linux/list.h>
> #include <linux/io.h>
> #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -603,6 +604,15 @@ static int c_can_start(struct net_device *dev)
>
> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>
> + /* activate pins */
> + if (!IS_ERR(priv->pinctrl)) {
> + struct pinctrl_state *s;
> +
> + s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
> + if (!IS_ERR(s))
> + pinctrl_select_state(priv->pinctrl, s);
> + }
Can you factor this into a common function? Which is used like this:
c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT)
Otherwise looks god.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function
From: Or Gerlitz @ 2014-11-05 13:30 UTC (permalink / raw)
To: Eyal perry, Eyal Perry
Cc: Amir Vadai, David S. Miller, Ben Hutchings, netdev,
Yevgeny Petrilin
In-Reply-To: <545A20B0.4080807@dev.mellanox.co.il>
On 11/5/2014 3:05 PM, Eyal perry wrote:
> On 11/5/2014 2:27 PM, Or Gerlitz wrote:
>>> +
>>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>>> +{
>>> + struct mlx4_en_priv *priv = netdev_priv(dev);
>>> + struct mlx4_en_dev *mdev = priv->mdev;
>>> + u32 prev_rss_hash_fn = priv->rss_hash_fn;
>>> + int err = 0;
>>> +
>>> + if (!(hfunc & priv->rss_hash_fn_caps))
>>> + return -EINVAL;
>>> +
>>> + priv->rss_hash_fn = hfunc;
>> Seems that you are blindly setting here priv->rss_hash_fn to the function (xor or top) requested by the user w.o prior checking if the firmware actually supports that
>> (e.g test it against priv->rss_hash_fn_caps, right?
>>
> That exactly what I do 3 lines above, priv->rss_hash_fn_caps is derived from firmware capabilities.
got it, thanks.
>
> [...]
>
>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>>> mlx4_en_priv *priv)
>>> }
>>> rss_context->flags = rss_mask;
>>> - rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>> - for (i = 0; i < 10; i++)
>>> - rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>> -
>>> + if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>>> + rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>>> + } else {
>>> + rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>> + for (i = 0; i < 10; i++)
>>> + rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>> + }
>> So this patch effectively changes the default from top to xor, right? is
>> that intentional? if yes, spare few words on the change log.
>>
> No, in general the default shouldn't be affected by the patch (unless
> there's a bug). The default value is set in mlx4_en_init_netdev() and it
> will remain Toeplitz unless firmware is supports only XOR hash function.
>
Oh, I see why I was confused and what I think need to be fixed. You are
using both MLX4_RSS_HASH_TOP/XOR and RSS_HASH_TOP/XOR, any reason for that?
Also, priv->rss_hash_fn should equal one value, right? so this code "if
(priv->rss_hash_fn & something)" is confusing, should be "if
(priv->rss_hash_fn == something)"
Or.
^ permalink raw reply
* Re: [PATCH v3 7/8] net: can: c_can: Add support for TI DRA7 DCAN
From: Marc Kleine-Budde @ 2014-11-05 13:30 UTC (permalink / raw)
To: Roger Quadros, wg
Cc: wsa, tony, tglx, mugunthanvnm, george.cherian, balbi, nsekhar, nm,
sergei.shtylyov, linux-omap, linux-can, netdev
In-Reply-To: <1415096461-25576-8-git-send-email-rogerq@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3090 bytes --]
On 11/04/2014 11:21 AM, Roger Quadros wrote:
> DRA7 SoC has 2 CAN IPs. Provide compatible IDs and RAMINIT
> register data for both.
My understanding of the discussion with Wolfram was:
- We should put the number of the Interface into to DT as a regmap
parameter.
- We put the method how to find the correct bits into the DT, via the
compatible.
So for both CAN instances on the DRA7 we have a single compatible
"ti,dra7-d_can" and in the driver a mechanism that translates the number
of the instance into the needed bit offsets, e.g. via two arrays.
Same comments for patch 8/8.
Marc
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> Documentation/devicetree/bindings/net/can/c_can.txt | 1 +
> drivers/net/can/c_can/c_can_platform.c | 16 ++++++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
> index 917ac0e..746cc07 100644
> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
> @@ -4,6 +4,7 @@ Bosch C_CAN/D_CAN controller Device Tree Bindings
> Required properties:
> - compatible : Should be "bosch,c_can" for C_CAN controllers and
> "bosch,d_can" for D_CAN controllers.
> + Can be "ti,dra7-d_can1" or "ti,dra7-d_can2".
> - reg : physical base address and size of the C_CAN/D_CAN
> registers map
> - interrupts : property with a value describing the interrupt
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index d058820..dc618ce 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -195,6 +195,20 @@ static struct c_can_driver_data d_can_drvdata = {
> .id = BOSCH_D_CAN,
> };
>
> +static struct c_can_driver_data dra7_dcan1_drvdata = {
> + .id = BOSCH_D_CAN,
> + .raminit_start_bit = 3,
> + .raminit_done_bit = 1,
> + .raminit_pulse = true,
> +};
> +
> +static struct c_can_driver_data dra7_dcan2_drvdata = {
> + .id = BOSCH_D_CAN,
> + .raminit_start_bit = 5,
> + .raminit_done_bit = 2,
> + .raminit_pulse = true,
> +};
> +
> static struct platform_device_id c_can_id_table[] = {
> {
> .name = KBUILD_MODNAME,
> @@ -215,6 +229,8 @@ MODULE_DEVICE_TABLE(platform, c_can_id_table);
> static const struct of_device_id c_can_of_table[] = {
> { .compatible = "bosch,c_can", .data = &c_can_drvdata },
> { .compatible = "bosch,d_can", .data = &d_can_drvdata },
> + { .compatible = "ti,dra7-d_can1", .data = &dra7_dcan1_drvdata },
> + { .compatible = "ti,dra7-d_can2", .data = &dra7_dcan2_drvdata },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, c_can_of_table);
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 6/8] net: can: c_can: Disable pins when CAN interface is down
From: Roger Quadros @ 2014-11-05 13:33 UTC (permalink / raw)
To: Marc Kleine-Budde, wg
Cc: wsa, tony, tglx, mugunthanvnm, george.cherian, balbi, nsekhar, nm,
sergei.shtylyov, linux-omap, linux-can, netdev
In-Reply-To: <545A2511.2090205@pengutronix.de>
On 11/05/2014 03:24 PM, Marc Kleine-Budde wrote:
> On 11/04/2014 11:20 AM, Roger Quadros wrote:
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> drivers/net/can/c_can/c_can.c | 20 ++++++++++++++++++++
>> drivers/net/can/c_can/c_can.h | 1 +
>> drivers/net/can/c_can/c_can_platform.c | 20 ++++++++++++++++++++
>> 3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 8e78bb4..4dfc3ce 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -35,6 +35,7 @@
>> #include <linux/list.h>
>> #include <linux/io.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>> #include <linux/can.h>
>> #include <linux/can/dev.h>
>> @@ -603,6 +604,15 @@ static int c_can_start(struct net_device *dev)
>>
>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>> + /* activate pins */
>> + if (!IS_ERR(priv->pinctrl)) {
>> + struct pinctrl_state *s;
>> +
>> + s = pinctrl_lookup_state(priv->pinctrl, PINCTRL_STATE_DEFAULT);
>> + if (!IS_ERR(s))
>> + pinctrl_select_state(priv->pinctrl, s);
>> + }
>
> Can you factor this into a common function? Which is used like this:
>
> c_can_pinctrl_select_state(priv, PINCTRL_STATE_DEFAULT)
>
> Otherwise looks god.
OK.
cheers,
-roger
^ permalink raw reply
* Re: [PATCH v3 7/8] net: can: c_can: Add support for TI DRA7 DCAN
From: Roger Quadros @ 2014-11-05 13:36 UTC (permalink / raw)
To: Marc Kleine-Budde, wg
Cc: wsa, tony, tglx, mugunthanvnm, george.cherian, balbi, nsekhar, nm,
sergei.shtylyov, linux-omap, linux-can, netdev
In-Reply-To: <545A2679.2000905@pengutronix.de>
On 11/05/2014 03:30 PM, Marc Kleine-Budde wrote:
> On 11/04/2014 11:21 AM, Roger Quadros wrote:
>> DRA7 SoC has 2 CAN IPs. Provide compatible IDs and RAMINIT
>> register data for both.
>
> My understanding of the discussion with Wolfram was:
> - We should put the number of the Interface into to DT as a regmap
> parameter.
> - We put the method how to find the correct bits into the DT, via the
> compatible.
>
> So for both CAN instances on the DRA7 we have a single compatible
> "ti,dra7-d_can" and in the driver a mechanism that translates the number
> of the instance into the needed bit offsets, e.g. via two arrays.
>
OK. I'll revise this series.
The new syscon-raminit property will be like
syscon-raminit = <syscon_phandle raminit-reg-offset dcan-interface-number>;
cheers,
-roger
> Same comments for patch 8/8.
>
> Marc
>
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> Documentation/devicetree/bindings/net/can/c_can.txt | 1 +
>> drivers/net/can/c_can/c_can_platform.c | 16 ++++++++++++++++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/c_can.txt b/Documentation/devicetree/bindings/net/can/c_can.txt
>> index 917ac0e..746cc07 100644
>> --- a/Documentation/devicetree/bindings/net/can/c_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/c_can.txt
>> @@ -4,6 +4,7 @@ Bosch C_CAN/D_CAN controller Device Tree Bindings
>> Required properties:
>> - compatible : Should be "bosch,c_can" for C_CAN controllers and
>> "bosch,d_can" for D_CAN controllers.
>> + Can be "ti,dra7-d_can1" or "ti,dra7-d_can2".
>> - reg : physical base address and size of the C_CAN/D_CAN
>> registers map
>> - interrupts : property with a value describing the interrupt
>> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
>> index d058820..dc618ce 100644
>> --- a/drivers/net/can/c_can/c_can_platform.c
>> +++ b/drivers/net/can/c_can/c_can_platform.c
>> @@ -195,6 +195,20 @@ static struct c_can_driver_data d_can_drvdata = {
>> .id = BOSCH_D_CAN,
>> };
>>
>> +static struct c_can_driver_data dra7_dcan1_drvdata = {
>> + .id = BOSCH_D_CAN,
>> + .raminit_start_bit = 3,
>> + .raminit_done_bit = 1,
>> + .raminit_pulse = true,
>> +};
>> +
>> +static struct c_can_driver_data dra7_dcan2_drvdata = {
>> + .id = BOSCH_D_CAN,
>> + .raminit_start_bit = 5,
>> + .raminit_done_bit = 2,
>> + .raminit_pulse = true,
>> +};
>> +
>> static struct platform_device_id c_can_id_table[] = {
>> {
>> .name = KBUILD_MODNAME,
>> @@ -215,6 +229,8 @@ MODULE_DEVICE_TABLE(platform, c_can_id_table);
>> static const struct of_device_id c_can_of_table[] = {
>> { .compatible = "bosch,c_can", .data = &c_can_drvdata },
>> { .compatible = "bosch,d_can", .data = &d_can_drvdata },
>> + { .compatible = "ti,dra7-d_can1", .data = &dra7_dcan1_drvdata },
>> + { .compatible = "ti,dra7-d_can2", .data = &dra7_dcan2_drvdata },
>> { /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, c_can_of_table);
>>
>
>
^ permalink raw reply
* Re: [PATCH v3 7/8] net: can: c_can: Add support for TI DRA7 DCAN
From: Marc Kleine-Budde @ 2014-11-05 13:43 UTC (permalink / raw)
To: Roger Quadros, wg
Cc: wsa, tony, tglx, mugunthanvnm, george.cherian, balbi, nsekhar, nm,
sergei.shtylyov, linux-omap, linux-can, netdev
In-Reply-To: <545A27E8.9090600@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]
On 11/05/2014 02:36 PM, Roger Quadros wrote:
> On 11/05/2014 03:30 PM, Marc Kleine-Budde wrote:
>> On 11/04/2014 11:21 AM, Roger Quadros wrote:
>>> DRA7 SoC has 2 CAN IPs. Provide compatible IDs and RAMINIT
>>> register data for both.
>>
>> My understanding of the discussion with Wolfram was:
>> - We should put the number of the Interface into to DT as a regmap
>> parameter.
>> - We put the method how to find the correct bits into the DT, via the
>> compatible.
>>
>> So for both CAN instances on the DRA7 we have a single compatible
>> "ti,dra7-d_can" and in the driver a mechanism that translates the number
>> of the instance into the needed bit offsets, e.g. via two arrays.
>>
>
> OK. I'll revise this series.
>
> The new syscon-raminit property will be like
> syscon-raminit = <syscon_phandle raminit-reg-offset dcan-interface-number>;
Yes. Wolfram?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Dong Aisheng @ 2014-11-05 13:46 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Oliver Hartkopp, linux-can, wg, varkabhadram, netdev,
linux-arm-kernel
In-Reply-To: <545A23DA.7030303@pengutronix.de>
On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote:
> On 11/05/2014 02:10 PM, Oliver Hartkopp wrote:
> > On 05.11.2014 12:26, Dong Aisheng wrote:
> >> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote:
> >>> On 05.11.2014 08:58, Dong Aisheng wrote:
> >
> >
> >>> Unfortunately No. Here it becomes complicated due to the fact that
> >>> the revision 3.0.x does not support per-frame switching for FD/BRS
> >>> ...
> >>
> >> I'm not sure i got your point.
> >> From m_can spec, it allows switch CAN mode by setting CMR bit.
> >>
> >> Bits 11:10 CMR[1:0]: CAN Mode Request
> >> A change of the CAN operation mode is requested by writing to this bit
> >> field. After change to the
> >> requested operation mode the bit field is reset to “00” and the status
> >> flags FDBS and FDO are set
> >> accordingly. In case the requested CAN operation mode is not enabled,
> >> the value written to CMR is
> >> retained until it is overwritten by the next mode change request. In
> >> case CME = “01”/”10”/”11” a
> >> change to CAN operation according to ISO 11898-1 is always possible.
> >> Default is CAN operation
> >> according to ISO11898-1.
> >> 00 unchanged
> >> 01 Request CAN FD operation
> >> 10 Request CAN FD operation with bit rate switching
> >> 11 Request CAN operation according ISO11898-1
> >>
> >> So what's the difference between this function and the per-frame
> >> switching
> >> you mentioned?
> >>
> >>>
> >>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only*
> >>> tells us, that the controller is _capable_ to send either CAN or CAN
> >>> FD frames.
> >>>
> >>> It does not configure the controller into one of these specific
> >>> settings!
> >>>
> >>> Additionally: AFAIK when writing to the CCCR you have to make sure
> >>> that there's currently no ongoing transfer.
> >>>
> >>
> >> I don't know about it before.
> >> By searching m_can user manual v302 again, i still did not find this
> >> limitation. Can you point me if you know where it is?
> >>
> >> BTW, since we only use one Tx Buffer for transmission currently, so we
> >> should not meet that case that CAN mode is switched during transfer.
> >> So the issue you concern may not happen.
> >
> > Yes. You are right. Having a FIFO with a size of 1 will help here :-)
>
> Errrr....sorry...no.
>
> Taking an easy route here but making it x times harder to extend the
> driver to make use of the FIFO is not an option.
>
Hmm, this way is just following the original approach the current driver
used. It's initial work and won't make things complicated.
Extend the driver to use FIFO for TX is another story and based on
my understanding it may be a bit complicated to do CAN FD mode switch on this
case due to hw limitation that the revision 3.0.x does not support per-frame
switching for FD/BRS as Oliver pointed out.
(e.g. how to switch FD MODE for each frame on Tx FIFO?)
Probably that's why the 3.1.x version will add the FD/BRS bit controller
in Tx Buffer to fix this issue.
Anyway, that's future work and we can discuss it when adding FIFO support
for Tx function.
Regards
Dong Aisheng
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
^ permalink raw reply
* Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function
From: Eyal perry @ 2014-11-05 13:48 UTC (permalink / raw)
To: Or Gerlitz, Eyal Perry
Cc: Amir Vadai, David S. Miller, Ben Hutchings, netdev,
Yevgeny Petrilin
In-Reply-To: <545A2664.7040001@mellanox.com>
On 11/5/2014 3:30 PM, Or Gerlitz wrote:
> On 11/5/2014 3:05 PM, Eyal perry wrote:
>> On 11/5/2014 2:27 PM, Or Gerlitz wrote:
>>>> +
>>>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>>>> +{
>>>> + struct mlx4_en_priv *priv = netdev_priv(dev);
>>>> + struct mlx4_en_dev *mdev = priv->mdev;
>>>> + u32 prev_rss_hash_fn = priv->rss_hash_fn;
>>>> + int err = 0;
>>>> +
>>>> + if (!(hfunc & priv->rss_hash_fn_caps))
>>>> + return -EINVAL;
>>>> +
>>>> + priv->rss_hash_fn = hfunc;
>>> Seems that you are blindly setting here priv->rss_hash_fn to the
>>> function (xor or top) requested by the user w.o prior checking if the
>>> firmware actually supports that
>>> (e.g test it against priv->rss_hash_fn_caps, right?
>>>
>> That exactly what I do 3 lines above, priv->rss_hash_fn_caps is
>> derived from firmware capabilities.
>
> got it, thanks.
>
>>
>> [...]
>>
>>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>>>> mlx4_en_priv *priv)
>>>> }
>>>> rss_context->flags = rss_mask;
>>>> - rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> - for (i = 0; i < 10; i++)
>>>> - rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> -
>>>> + if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>>>> + rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>>>> + } else {
>>>> + rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> + for (i = 0; i < 10; i++)
>>>> + rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> + }
>>> So this patch effectively changes the default from top to xor, right? is
>>> that intentional? if yes, spare few words on the change log.
>>>
>> No, in general the default shouldn't be affected by the patch (unless
>> there's a bug). The default value is set in mlx4_en_init_netdev() and it
>> will remain Toeplitz unless firmware is supports only XOR hash function.
>>
>
> Oh, I see why I was confused and what I think need to be fixed. You are
> using both MLX4_RSS_HASH_TOP/XOR and RSS_HASH_TOP/XOR, any reason for that?
Yes, RSS_HASH_TOP/XOR are bits on a generic bitmask while the
MLX4_RSS_HASH_TOP/XOR are HW specific values.
>
> Also, priv->rss_hash_fn should equal one value, right? so this code "if
> (priv->rss_hash_fn & something)" is confusing, should be "if
> (priv->rss_hash_fn == something)"
That's right, I'll fix it for V1.
Thanks.
>
> Or.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function
From: Or Gerlitz @ 2014-11-05 13:53 UTC (permalink / raw)
To: Eyal Perry
Cc: Amir Vadai, David S. Miller, Ben Hutchings, netdev,
Yevgeny Petrilin
In-Reply-To: <1415188769-19593-3-git-send-email-amirv@mellanox.com>
On 11/5/2014 1:59 PM, Amir Vadai wrote:
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2585,6 +2585,15 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
> dev->features |= NETIF_F_GSO_UDP_TUNNEL;
> }
>
> + if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR) {
> + priv->rss_hash_fn_caps |= RSS_HASH_XOR;
> + priv->rss_hash_fn = RSS_HASH_XOR;
> + }
> + if (mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP) {
> + priv->rss_hash_fn_caps |= RSS_HASH_TOP;
> + priv->rss_hash_fn = RSS_HASH_TOP;
> + }
> +
can we somehow change this code to be more clear and assign
priv->rss_hash_fn once?
> mdev->pndev[port] = dev;
>
> netif_carrier_off(dev);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox