* [PATCH net v3] ipv6: mld: fix add_grhead skb_over_panic for devs with large MTUs
From: Daniel Borkmann @ 2014-11-05 14:42 UTC (permalink / raw)
To: davem; +Cc: lw1a2.jing, fw, hannes, netdev, Eric Dumazet, David L Stevens
It has been reported that generating an MLD listener report on
devices with large MTUs (e.g. 9000) and a high number of IPv6
addresses can trigger a skb_over_panic():
skbuff: skb_over_panic: text:ffffffff80612a5d len:3776 put:20
head:ffff88046d751000 data:ffff88046d751010 tail:0xed0 end:0xec0
dev:port1
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:100!
invalid opcode: 0000 [#1] SMP
Modules linked in: ixgbe(O)
CPU: 3 PID: 0 Comm: swapper/3 Tainted: G O 3.14.23+ #4
[...]
Call Trace:
<IRQ>
[<ffffffff80578226>] ? skb_put+0x3a/0x3b
[<ffffffff80612a5d>] ? add_grhead+0x45/0x8e
[<ffffffff80612e3a>] ? add_grec+0x394/0x3d4
[<ffffffff80613222>] ? mld_ifc_timer_expire+0x195/0x20d
[<ffffffff8061308d>] ? mld_dad_timer_expire+0x45/0x45
[<ffffffff80255b5d>] ? call_timer_fn.isra.29+0x12/0x68
[<ffffffff80255d16>] ? run_timer_softirq+0x163/0x182
[<ffffffff80250e6f>] ? __do_softirq+0xe0/0x21d
[<ffffffff8025112b>] ? irq_exit+0x4e/0xd3
[<ffffffff802214bb>] ? smp_apic_timer_interrupt+0x3b/0x46
[<ffffffff8063f10a>] ? apic_timer_interrupt+0x6a/0x70
mld_newpack() skb allocations are usually requested with dev->mtu
in size, since commit 72e09ad107e7 ("ipv6: avoid high order allocations")
we have changed the limit in order to be less likely to fail.
However, in MLD/IGMP code, we have some rather ugly AVAILABLE(skb)
macros, which determine if we may end up doing an skb_put() for
adding another record. To avoid possible fragmentation, we check
the skb's tailroom as skb->dev->mtu - skb->len, which is a wrong
assumption as the actual max allocation size can be much smaller.
The IGMP case doesn't have this issue as commit 57e1ab6eaddc
("igmp: refine skb allocations") stores the allocation size in
the cb[].
Set a reserved_tailroom to make it fit into the MTU and use
skb_availroom() helper instead. This also allows to get rid of
igmp_skb_size().
Reported-by: Wei Liu <lw1a2.jing@gmail.com>
Fixes: 72e09ad107e7 ("ipv6: avoid high order allocations")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: David L Stevens <david.stevens@oracle.com>
---
v2->v3:
- Still had a discussion w/ Hannes and improved the code a bit to
make it more clear to read
v1->v2:
- Don't introduce skb_nofrag_tailroom(), but reuse skb_availroom()
as suggested by Eric
net/ipv4/igmp.c | 17 +++++++----------
net/ipv6/mcast.c | 19 ++++++++++---------
2 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index fb70e3e..d90bdbf 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -318,9 +318,7 @@ igmp_scount(struct ip_mc_list *pmc, int type, int gdeleted, int sdeleted)
return scount;
}
-#define igmp_skb_size(skb) (*(unsigned int *)((skb)->cb))
-
-static struct sk_buff *igmpv3_newpack(struct net_device *dev, int size)
+static struct sk_buff *igmpv3_newpack(struct net_device *dev, unsigned int mtu)
{
struct sk_buff *skb;
struct rtable *rt;
@@ -330,6 +328,7 @@ static struct sk_buff *igmpv3_newpack(struct net_device *dev, int size)
struct flowi4 fl4;
int hlen = LL_RESERVED_SPACE(dev);
int tlen = dev->needed_tailroom;
+ unsigned int size = mtu;
while (1) {
skb = alloc_skb(size + hlen + tlen,
@@ -340,20 +339,19 @@ static struct sk_buff *igmpv3_newpack(struct net_device *dev, int size)
if (size < 256)
return NULL;
}
- skb->priority = TC_PRIO_CONTROL;
- igmp_skb_size(skb) = size;
rt = ip_route_output_ports(net, &fl4, NULL, IGMPV3_ALL_MCR, 0,
- 0, 0,
- IPPROTO_IGMP, 0, dev->ifindex);
+ 0, 0, IPPROTO_IGMP, 0, dev->ifindex);
if (IS_ERR(rt)) {
kfree_skb(skb);
return NULL;
}
+ skb->priority = TC_PRIO_CONTROL;
skb_dst_set(skb, &rt->dst);
skb->dev = dev;
-
+ skb->reserved_tailroom = skb_end_offset(skb) -
+ min(mtu, skb_end_offset(skb));
skb_reserve(skb, hlen);
skb_reset_network_header(skb);
@@ -423,8 +421,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ip_mc_list *pmc,
return skb;
}
-#define AVAILABLE(skb) ((skb) ? ((skb)->dev ? igmp_skb_size(skb) - (skb)->len : \
- skb_tailroom(skb)) : 0)
+#define AVAILABLE(skb) ((skb) ? skb_availroom(skb) : 0)
static struct sk_buff *add_grec(struct sk_buff *skb, struct ip_mc_list *pmc,
int type, int gdeleted, int sdeleted)
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 9648de2..d817737 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1550,7 +1550,7 @@ static void ip6_mc_hdr(struct sock *sk, struct sk_buff *skb,
hdr->daddr = *daddr;
}
-static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
+static struct sk_buff *mld_newpack(struct inet6_dev *idev, unsigned int mtu)
{
struct net_device *dev = idev->dev;
struct net *net = dev_net(dev);
@@ -1560,22 +1560,23 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
struct in6_addr addr_buf;
const struct in6_addr *saddr;
int hlen = LL_RESERVED_SPACE(dev);
- int tlen = dev->needed_tailroom;
- int err;
+ int err, tlen = dev->needed_tailroom;
+ unsigned int size = mtu + hlen + tlen;
u8 ra[8] = { IPPROTO_ICMPV6, 0,
IPV6_TLV_ROUTERALERT, 2, 0, 0,
IPV6_TLV_PADN, 0 };
- /* we assume size > sizeof(ra) here */
- size += hlen + tlen;
- /* limit our allocations to order-0 page */
+ /* We assume size > sizeof(ra) here. Limit our
+ * allocations to order-0 page.
+ */
size = min_t(int, size, SKB_MAX_ORDER(0, 0));
skb = sock_alloc_send_skb(sk, size, 1, &err);
-
if (!skb)
return NULL;
skb->priority = TC_PRIO_CONTROL;
+ skb->reserved_tailroom = skb_end_offset(skb) -
+ min(mtu, skb_end_offset(skb));
skb_reserve(skb, hlen);
if (__ipv6_get_lladdr(idev, &addr_buf, IFA_F_TENTATIVE)) {
@@ -1599,6 +1600,7 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, int size)
pmr->mld2r_cksum = 0;
pmr->mld2r_resv2 = 0;
pmr->mld2r_ngrec = 0;
+
return skb;
}
@@ -1690,8 +1692,7 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
return skb;
}
-#define AVAILABLE(skb) ((skb) ? ((skb)->dev ? (skb)->dev->mtu - (skb)->len : \
- skb_tailroom(skb)) : 0)
+#define AVAILABLE(skb) ((skb) ? skb_availroom(skb) : 0)
static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
int type, int gdeleted, int sdeleted, int crsend)
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH V3 2/3] can: m_can: update to support CAN FD features
From: Oliver Hartkopp @ 2014-11-05 14:42 UTC (permalink / raw)
To: Marc Kleine-Budde, Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <545A34B9.2060001@pengutronix.de>
Looks good to me now.
Thanks for your patience.
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
On 05.11.2014 15:31, Marc Kleine-Budde wrote:
> On 11/05/2014 02:16 PM, Dong Aisheng wrote:
>> Bosch M_CAN is CAN FD capable device. This patch implements the CAN
>> FD features include up to 64 bytes payload and bitrate switch function.
>> 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
>> up to 64 bytes payload. It's backward compatible with old 8 bytes
>> normal CAN frame.
>> 2) Allocate can frame or canfd frame based on EDL bit
>> 3) Bitrate Switch function is disabled by default and will be enabled
>> according to CANFD_BRS bit in cf->flags.
>>
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>
> I'm waiting on Oliver's Ack on this patch, then I'll apply 1+2.
>
> regards,
> Marc
>
^ permalink raw reply
* Re: [PATCH V2 1/4] can: m_can: update to support CAN FD features
From: Oliver Hartkopp @ 2014-11-05 14:35 UTC (permalink / raw)
To: Dong Aisheng, Marc Kleine-Budde
Cc: linux-can, wg, varkabhadram, netdev, linux-arm-kernel
In-Reply-To: <20141105134625.GG4007@shlinux1.ap.freescale.net>
On 05.11.2014 14:46, Dong Aisheng wrote:
> 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 th
> 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 |
>>
>
>
at 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.
>
Yes. I have to second this opinion.
I also would like to have a TX FIFO. But due to the limitations of the 3.0.x
M_CAN I would suggest to prefer a 'correct" CAN FD driver implementation in
favor of having a TX FIFO which is unusable for mixed CAN frame types.
Let's try the FIFO stuff with the next M_CAN revision.
It's a bit of a SJA1000 for CAN FD :-)
Regards,
Oliver
^ permalink raw reply
* [PATCH] net: mv643xx_eth: reclaim TX skbs only when released by the HW
From: Karl Beldan @ 2014-11-05 14:32 UTC (permalink / raw)
To: David Miller
Cc: Karl Beldan, netdev, Ian Campbell, Eric Dumazet, Ezequiel Garcia,
Sebastian Hesselbarth
From: Karl Beldan <karl.beldan@rivierawaves.com>
ATM, txq_reclaim will dequeue and free an skb for each tx desc released
by the hw that has TX_LAST_DESC set. However, in case of TSO, each
hw desc embedding the last part of a segment has TX_LAST_DESC set,
losing the one-to-one 'last skb frag'/'TX_LAST_DESC set' correspondance,
which causes data corruption.
Fix this by checking TX_ENABLE_INTERRUPT instead of TX_LAST_DESC, and
warn when trying to dequeue from an empty txq (which can be symptomatic
of releasing skbs prematurely).
Fixes: 3ae8f4e0b98 ('net: mv643xx_eth: Implement software TSO')
Reported-by: Slawomir Gajzner <slawomir.gajzner@gmail.com>
Reported-by: Julien D'Ascenzio <jdascenzio@yahoo.fr>
Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
Cc: Ian Campbell <ijc@hellion.org.uk>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Ian, I refrained myself from embedding your Tested-by since this change is a
little bit different from my first.
David, please consider queueing this one up for -stable, and possibly adjust
Ian's Tested-by (I am not sure about the process).
drivers/net/ethernet/marvell/mv643xx_eth.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index b151a94..d44560d 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1047,7 +1047,6 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
int tx_index;
struct tx_desc *desc;
u32 cmd_sts;
- struct sk_buff *skb;
tx_index = txq->tx_used_desc;
desc = &txq->tx_desc_area[tx_index];
@@ -1066,19 +1065,22 @@ static int txq_reclaim(struct tx_queue *txq, int budget, int force)
reclaimed++;
txq->tx_desc_count--;
- skb = NULL;
- if (cmd_sts & TX_LAST_DESC)
- skb = __skb_dequeue(&txq->tx_skb);
+ if (!IS_TSO_HEADER(txq, desc->buf_ptr))
+ dma_unmap_single(mp->dev->dev.parent, desc->buf_ptr,
+ desc->byte_cnt, DMA_TO_DEVICE);
+
+ if (cmd_sts & TX_ENABLE_INTERRUPT) {
+ struct sk_buff *skb = __skb_dequeue(&txq->tx_skb);
+
+ if (!WARN_ON(!skb))
+ dev_kfree_skb(skb);
+ }
if (cmd_sts & ERROR_SUMMARY) {
netdev_info(mp->dev, "tx error\n");
mp->dev->stats.tx_errors++;
}
- if (!IS_TSO_HEADER(txq, desc->buf_ptr))
- dma_unmap_single(mp->dev->dev.parent, desc->buf_ptr,
- desc->byte_cnt, DMA_TO_DEVICE);
- dev_kfree_skb(skb);
}
__netif_tx_unlock_bh(nq);
--
2.0.1
^ permalink raw reply related
* Re: [PATCH V3 2/3] can: m_can: update to support CAN FD features
From: Marc Kleine-Budde @ 2014-11-05 14:31 UTC (permalink / raw)
To: Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <1415193393-30023-2-git-send-email-b29396@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 952 bytes --]
On 11/05/2014 02:16 PM, Dong Aisheng wrote:
> Bosch M_CAN is CAN FD capable device. This patch implements the CAN
> FD features include up to 64 bytes payload and bitrate switch function.
> 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
> up to 64 bytes payload. It's backward compatible with old 8 bytes
> normal CAN frame.
> 2) Allocate can frame or canfd frame based on EDL bit
> 3) Bitrate Switch function is disabled by default and will be enabled
> according to CANFD_BRS bit in cf->flags.
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
I'm waiting on Oliver's Ack on this patch, then I'll apply 1+2.
regards,
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 3/3] can: m_can: workaround for transmit data less than 4 bytes
From: Marc Kleine-Budde @ 2014-11-05 14:29 UTC (permalink / raw)
To: Dong Aisheng, linux-can
Cc: wg, varkabhadram, netdev, socketcan, linux-arm-kernel
In-Reply-To: <1415193393-30023-3-git-send-email-b29396@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]
On 11/05/2014 02:16 PM, Dong Aisheng wrote:
> At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1, an issue with
> the Message RAM was discovered. Sending CAN frames with dlc less
> than 4 bytes will lead to bit errors, when the first 8 bytes of
> the Message RAM have not been initialized (i.e. written to).
> To work around this issue, the first 8 bytes are initialized in open()
> function.
>
> Without the workaround, we can easily see the following errors:
> root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> [ 66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@imx6qdlsolo:~# cansend can0 123#112233
> [ 66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
>
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
Applied to can/master
Thanks,
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
* [PATCH V3 3/3] can: m_can: workaround for transmit data less than 4 bytes
From: Dong Aisheng @ 2014-11-05 13:16 UTC (permalink / raw)
To: linux-can
Cc: mkl, wg, varkabhadram, netdev, socketcan, b29396,
linux-arm-kernel
In-Reply-To: <1415193393-30023-1-git-send-email-b29396@freescale.com>
At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1, an issue with
the Message RAM was discovered. Sending CAN frames with dlc less
than 4 bytes will lead to bit errors, when the first 8 bytes of
the Message RAM have not been initialized (i.e. written to).
To work around this issue, the first 8 bytes are initialized in open()
function.
Without the workaround, we can easily see the following errors:
root@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
[ 66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@imx6qdlsolo:~# cansend can0 123#112233
[ 66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangeLog
v2->v3:
* add i.MX chip version in issue in commit message
v1->v2:
* initialize the first 8 bytes of Tx Buffer of Message RAM in open()
to workaround the issue
---
drivers/net/can/m_can/m_can.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index eee1533..567cd27 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -905,6 +905,16 @@ static void m_can_chip_config(struct net_device *dev)
/* set bittiming params */
m_can_set_bittiming(dev);
+ /* At least on the i.MX6SX TO1.2 with M_CAN IP version 3.0.1,
+ * (CREL = 30130506) an issue with the Message RAM was discovered.
+ * Sending CAN frames with dlc less than 4 bytes will lead to bit
+ * errors, when the first 8 bytes of the Message RAM have not been
+ * initialized (i.e. written to). To work around this issue, the
+ * first 8 bytes are initialized here.
+ */
+ m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
+ m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
+
m_can_config_endisable(priv, false);
}
--
1.9.1
^ permalink raw reply related
* [PATCH V3 2/3] can: m_can: update to support CAN FD features
From: Dong Aisheng @ 2014-11-05 13:16 UTC (permalink / raw)
To: linux-can
Cc: mkl, wg, varkabhadram, netdev, socketcan, b29396,
linux-arm-kernel
In-Reply-To: <1415193393-30023-1-git-send-email-b29396@freescale.com>
Bosch M_CAN is CAN FD capable device. This patch implements the CAN
FD features include up to 64 bytes payload and bitrate switch function.
1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD
up to 64 bytes payload. It's backward compatible with old 8 bytes
normal CAN frame.
2) Allocate can frame or canfd frame based on EDL bit
3) Bitrate Switch function is disabled by default and will be enabled
according to CANFD_BRS bit in cf->flags.
Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
ChangeLog:
v2->v3:
* integrate patch 4 into patch 1 in last series(allow to send std frame
on can fd mode)
* use suitable API to get cf->len according to RX_BUF_EDL bit
v1->v2:
* Allocate can frame or canfd frame based on EDL bit
* Only check and set RTR bit for normal frame (no EDL bit set)
---
drivers/net/can/m_can/m_can.c | 179 ++++++++++++++++++++++++++++++++----------
1 file changed, 136 insertions(+), 43 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6160b9c..eee1533 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -105,14 +105,36 @@ enum m_can_mram_cfg {
MRAM_CFG_NUM,
};
+/* Fast Bit Timing & Prescaler Register (FBTP) */
+#define FBTR_FBRP_MASK 0x1f
+#define FBTR_FBRP_SHIFT 16
+#define FBTR_FTSEG1_SHIFT 8
+#define FBTR_FTSEG1_MASK (0xf << FBTR_FTSEG1_SHIFT)
+#define FBTR_FTSEG2_SHIFT 4
+#define FBTR_FTSEG2_MASK (0x7 << FBTR_FTSEG2_SHIFT)
+#define FBTR_FSJW_SHIFT 0
+#define FBTR_FSJW_MASK 0x3
+
/* Test Register (TEST) */
#define TEST_LBCK BIT(4)
/* CC Control Register(CCCR) */
-#define CCCR_TEST BIT(7)
-#define CCCR_MON BIT(5)
-#define CCCR_CCE BIT(1)
-#define CCCR_INIT BIT(0)
+#define CCCR_TEST BIT(7)
+#define CCCR_CMR_MASK 0x3
+#define CCCR_CMR_SHIFT 10
+#define CCCR_CMR_CANFD 0x1
+#define CCCR_CMR_CANFD_BRS 0x2
+#define CCCR_CMR_CAN 0x3
+#define CCCR_CME_MASK 0x3
+#define CCCR_CME_SHIFT 8
+#define CCCR_CME_CAN 0
+#define CCCR_CME_CANFD 0x1
+#define CCCR_CME_CANFD_BRS 0x2
+#define CCCR_TEST BIT(7)
+#define CCCR_MON BIT(5)
+#define CCCR_CCE BIT(1)
+#define CCCR_INIT BIT(0)
+#define CCCR_CANFD 0x10
/* Bit Timing & Prescaler Register (BTP) */
#define BTR_BRP_MASK 0x3ff
@@ -204,6 +226,7 @@ enum m_can_mram_cfg {
/* Rx Buffer / FIFO Element Size Configuration (RXESC) */
#define M_CAN_RXESC_8BYTES 0x0
+#define M_CAN_RXESC_64BYTES 0x777
/* Tx Buffer Configuration(TXBC) */
#define TXBC_NDTB_OFF 16
@@ -211,6 +234,7 @@ enum m_can_mram_cfg {
/* Tx Buffer Element Size Configuration(TXESC) */
#define TXESC_TBDS_8BYTES 0x0
+#define TXESC_TBDS_64BYTES 0x7
/* Tx Event FIFO Con.guration (TXEFC) */
#define TXEFC_EFS_OFF 16
@@ -219,11 +243,11 @@ enum m_can_mram_cfg {
/* Message RAM Configuration (in bytes) */
#define SIDF_ELEMENT_SIZE 4
#define XIDF_ELEMENT_SIZE 8
-#define RXF0_ELEMENT_SIZE 16
-#define RXF1_ELEMENT_SIZE 16
+#define RXF0_ELEMENT_SIZE 72
+#define RXF1_ELEMENT_SIZE 72
#define RXB_ELEMENT_SIZE 16
#define TXE_ELEMENT_SIZE 8
-#define TXB_ELEMENT_SIZE 16
+#define TXB_ELEMENT_SIZE 72
/* Message RAM Elements */
#define M_CAN_FIFO_ID 0x0
@@ -231,11 +255,17 @@ enum m_can_mram_cfg {
#define M_CAN_FIFO_DATA(n) (0x8 + ((n) << 2))
/* Rx Buffer Element */
+/* R0 */
#define RX_BUF_ESI BIT(31)
#define RX_BUF_XTD BIT(30)
#define RX_BUF_RTR BIT(29)
+/* R1 */
+#define RX_BUF_ANMF BIT(31)
+#define RX_BUF_EDL BIT(21)
+#define RX_BUF_BRS BIT(20)
/* Tx Buffer Element */
+/* R0 */
#define TX_BUF_XTD BIT(30)
#define TX_BUF_RTR BIT(29)
@@ -327,41 +357,68 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv)
m_can_write(priv, M_CAN_ILE, 0x0);
}
-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf,
- u32 rxfs)
+static void m_can_read_fifo(struct net_device *dev, u32 rxfs)
{
+ struct net_device_stats *stats = &dev->stats;
struct m_can_priv *priv = netdev_priv(dev);
- u32 id, fgi;
+ struct canfd_frame *cf;
+ struct sk_buff *skb;
+ u32 id, fgi, dlc;
+ int i;
/* calculate the fifo get index for where to read data */
fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
+ dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
+ if (dlc & RX_BUF_EDL)
+ skb = alloc_canfd_skb(dev, &cf);
+ else
+ skb = alloc_can_skb(dev, (struct can_frame **)&cf);
+ if (!skb) {
+ stats->rx_dropped++;
+ return;
+ }
+
id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID);
if (id & RX_BUF_XTD)
cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
else
cf->can_id = (id >> 18) & CAN_SFF_MASK;
- if (id & RX_BUF_RTR) {
+ if (id & RX_BUF_ESI) {
+ cf->flags |= CANFD_ESI;
+ netdev_dbg(dev, "ESI Error\n");
+ }
+
+ if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) {
cf->can_id |= CAN_RTR_FLAG;
} else {
id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC);
- cf->can_dlc = get_can_dlc((id >> 16) & 0x0F);
- *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi,
- M_CAN_FIFO_DATA(0));
- *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi,
- M_CAN_FIFO_DATA(1));
+ if (dlc & RX_BUF_EDL)
+ cf->len = can_dlc2len((id >> 16) & 0x0F);
+ else
+ cf->len = get_can_dlc((id >> 16) & 0x0F);
+
+ if (id & RX_BUF_BRS)
+ cf->flags |= CANFD_BRS;
+
+ for (i = 0; i < cf->len; i += 4)
+ *(u32 *)(cf->data + i) =
+ m_can_fifo_read(priv, fgi,
+ M_CAN_FIFO_DATA(i / 4));
}
/* acknowledge rx fifo 0 */
m_can_write(priv, M_CAN_RXF0A, fgi);
+
+ stats->rx_packets++;
+ stats->rx_bytes += cf->len;
+
+ netif_receive_skb(skb);
}
static int m_can_do_rx_poll(struct net_device *dev, int quota)
{
struct m_can_priv *priv = netdev_priv(dev);
- struct net_device_stats *stats = &dev->stats;
- struct sk_buff *skb;
- struct can_frame *frame;
u32 pkts = 0;
u32 rxfs;
@@ -375,18 +432,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota)
if (rxfs & RXFS_RFL)
netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
- skb = alloc_can_skb(dev, &frame);
- if (!skb) {
- stats->rx_dropped++;
- return pkts;
- }
-
- m_can_read_fifo(dev, frame, rxfs);
-
- stats->rx_packets++;
- stats->rx_bytes += frame->can_dlc;
-
- netif_receive_skb(skb);
+ m_can_read_fifo(dev, rxfs);
quota--;
pkts++;
@@ -744,10 +790,23 @@ static const struct can_bittiming_const m_can_bittiming_const = {
.brp_inc = 1,
};
+static const struct can_bittiming_const m_can_data_bittiming_const = {
+ .name = KBUILD_MODNAME,
+ .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */
+ .tseg1_max = 16,
+ .tseg2_min = 1, /* Time segment 2 = phase_seg2 */
+ .tseg2_max = 8,
+ .sjw_max = 4,
+ .brp_min = 1,
+ .brp_max = 32,
+ .brp_inc = 1,
+};
+
static int m_can_set_bittiming(struct net_device *dev)
{
struct m_can_priv *priv = netdev_priv(dev);
const struct can_bittiming *bt = &priv->can.bittiming;
+ const struct can_bittiming *dbt = &priv->can.data_bittiming;
u16 brp, sjw, tseg1, tseg2;
u32 reg_btp;
@@ -758,7 +817,17 @@ static int m_can_set_bittiming(struct net_device *dev)
reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) |
(tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT);
m_can_write(priv, M_CAN_BTP, reg_btp);
- netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp);
+
+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+ brp = dbt->brp - 1;
+ sjw = dbt->sjw - 1;
+ tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
+ tseg2 = dbt->phase_seg2 - 1;
+ reg_btp = (brp << FBTR_FBRP_SHIFT) | (sjw << FBTR_FSJW_SHIFT) |
+ (tseg1 << FBTR_FTSEG1_SHIFT) |
+ (tseg2 << FBTR_FTSEG2_SHIFT);
+ m_can_write(priv, M_CAN_FBTP, reg_btp);
+ }
return 0;
}
@@ -778,8 +847,8 @@ static void m_can_chip_config(struct net_device *dev)
m_can_config_endisable(priv, true);
- /* RX Buffer/FIFO Element Size 8 bytes data field */
- m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES);
+ /* RX Buffer/FIFO Element Size 64 bytes data field */
+ m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_64BYTES);
/* Accept Non-matching Frames Into FIFO 0 */
m_can_write(priv, M_CAN_GFC, 0x0);
@@ -788,8 +857,8 @@ static void m_can_chip_config(struct net_device *dev)
m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) |
priv->mcfg[MRAM_TXB].off);
- /* only support 8 bytes firstly */
- m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES);
+ /* support 64 bytes payload */
+ m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_64BYTES);
m_can_write(priv, M_CAN_TXEFC, (1 << TXEFC_EFS_OFF) |
priv->mcfg[MRAM_TXE].off);
@@ -804,7 +873,8 @@ static void m_can_chip_config(struct net_device *dev)
RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off);
cccr = m_can_read(priv, M_CAN_CCCR);
- cccr &= ~(CCCR_TEST | CCCR_MON);
+ cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) |
+ (CCCR_CME_MASK << CCCR_CME_SHIFT));
test = m_can_read(priv, M_CAN_TEST);
test &= ~TEST_LBCK;
@@ -816,6 +886,9 @@ static void m_can_chip_config(struct net_device *dev)
test |= TEST_LBCK;
}
+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
+ cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT;
+
m_can_write(priv, M_CAN_CCCR, cccr);
m_can_write(priv, M_CAN_TEST, test);
@@ -880,11 +953,13 @@ static struct net_device *alloc_m_can_dev(void)
priv->dev = dev;
priv->can.bittiming_const = &m_can_bittiming_const;
+ priv->can.data_bittiming_const = &m_can_data_bittiming_const;
priv->can.do_set_mode = m_can_set_mode;
priv->can.do_get_berr_counter = m_can_get_berr_counter;
priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_BERR_REPORTING;
+ CAN_CTRLMODE_BERR_REPORTING |
+ CAN_CTRLMODE_FD;
return dev;
}
@@ -967,8 +1042,9 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct m_can_priv *priv = netdev_priv(dev);
- struct can_frame *cf = (struct can_frame *)skb->data;
- u32 id;
+ struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+ u32 id, cccr;
+ int i;
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -987,11 +1063,28 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
/* message ram configuration */
m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
- m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, cf->can_dlc << 16);
- m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
- m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
+ m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
+
+ for (i = 0; i < cf->len; i += 4)
+ m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
+ *(u32 *)(cf->data + i));
+
can_put_echo_skb(skb, dev, 0);
+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
+ cccr = m_can_read(priv, M_CAN_CCCR);
+ cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT);
+ if (can_is_canfd_skb(skb)) {
+ if (cf->flags & CANFD_BRS)
+ cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT;
+ else
+ cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT;
+ } else {
+ cccr |= CCCR_CMR_CAN << CCCR_CMR_SHIFT;
+ }
+ m_can_write(priv, M_CAN_CCCR, cccr);
+ }
+
/* enable first TX buffer to start transfer */
m_can_write(priv, M_CAN_TXBTIE, 0x1);
m_can_write(priv, M_CAN_TXBAR, 0x1);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next v4 0/4] netns: allow to identify peer netns
From: Nicolas Dichtel @ 2014-11-05 14:23 UTC (permalink / raw)
To: Eric W. Biederman
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, luto-kltTT9wpgjJwATOyAt5JVQ,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
cwang-xCSkyg8dI+0RB7SZvlqPiA, linux-api-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <87wq7g831b.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
Le 31/10/2014 20:14, Eric W. Biederman a écrit :
> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
>
>> Le 30/10/2014 19:41, Eric W. Biederman a écrit :
>>> Nicolas Dichtel <nicolas.dichtel@6wind.com> writes:
>>>
>>>> The goal of this serie is to be able to multicast netlink messages with an
>>>> attribute that identify a peer netns.
>>>> This is needed by the userland to interpret some informations contained in
>>>> netlink messages (like IFLA_LINK value, but also some other attributes in case
>>>> of x-netns netdevice (see also
>>>> http://thread.gmane.org/gmane.linux.network/315933/focus=316064 and
>>>> http://thread.gmane.org/gmane.linux.kernel.containers/28301/focus=4239)).
>>>>
>>>> Ids of peer netns are set by userland via a new genl messages. These ids are
>>>> stored per netns and are local (ie only valid in the netns where they are set).
>>>> To avoid allocating an int for each peer netns, I use idr_for_each() to retrieve
>>>> the id of a peer netns. Note that it will be possible to add a table (struct net
>>>> -> id) later to optimize this lookup if needed.
>>>>
>>>> Patch 1/4 introduces the netlink API mechanism to set and get these ids.
>>>> Patch 2/4 and 3/4 implements an example of how to use these ids in rtnetlink
>>>> messages. And patch 4/4 shows that the netlink messages can be symetric between
>>>> a GET and a SET.
>>>>
>>>> iproute2 patches are available, I can send them on demand.
>>>
>>> A quick reply. I think this patchset is in the right general direction.
>>> There are some oddball details that seem odd/awkward to me such as using
>>> genetlink instead of rtnetlink to get and set the ids, and not having
>>> ids if they are not set (that feels like a maintenance/usability challenge).
>> No problem to use rtnetlink, in fact, I hesitated.
>>
>> For the second point, I'm not sure to follow you: how to have an id, which will
>> not break migration, without asking the user to set it?
>
> We have that situtation with ifindex already. Basically the thought is
> to allow an id to be set, but also allow an id to be auto-generated if
> we use an namespace without an id being set.
If my understanding is correct, the difference is that we want to hide some
netns.
Do you think we can generate an id for each netns that does not have one and
relying on the fact that this id has no meaning unless you have a netns file
descriptor that allow you to get the id of this netns?
Regards,
Nicolas
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
^ 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
* 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 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 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 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 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: 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 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 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 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
* [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: 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
* 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 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
* [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 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
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